Remove Mutex from BaseChannel.

There's a bit of copy/pasted code in the channel code, which is
making moving network traffic consistently over to the network thread
a bit trickier than it needs to be, so I'm also updating variable
names used in Set[Local|Remote]Content_w to be more explicitly the same
and make it clear that the code is copy/pasted (and future updates can
consolidate more of it).

Also removing some code from the video/voice media channels that's
effectively dead code (vector + registration methods that aren't needed)

Bug: webrtc:12705
Change-Id: I2e14e69fbc489a64fc1e8899aaf1cfc979fe840b
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/215978
Reviewed-by: Sam Zackrisson <saza@google.com>
Reviewed-by: Niels Moller <nisse@webrtc.org>
Commit-Queue: Tommi <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33847}

media/engine/webrtc_video_engine.cc

@@ -39,7 +39,6 @@
 #include "rtc_base/logging.h"
 #include "rtc_base/numerics/safe_conversions.h"
 #include "rtc_base/strings/string_builder.h"
-#include "rtc_base/thread.h"
 #include "rtc_base/time_utils.h"
 #include "rtc_base/trace_event.h"
 
@@ -704,7 +703,7 @@ WebRtcVideoChannel::WebRtcVideoChannel(
     webrtc::VideoDecoderFactory* decoder_factory,
     webrtc::VideoBitrateAllocatorFactory* bitrate_allocator_factory)
     : VideoMediaChannel(config),
-      worker_thread_(rtc::Thread::Current()),
+      worker_thread_(call->worker_thread()),
       call_(call),
       unsignalled_ssrc_handler_(&default_unsignalled_ssrc_handler_),
       video_config_(config.video),
@@ -2066,7 +2065,7 @@ WebRtcVideoChannel::WebRtcVideoSendStream::WebRtcVideoSendStream(
     // TODO(deadbeef): Don't duplicate information between send_params,
     // rtp_extensions, options, etc.
     const VideoSendParameters& send_params)
-    : worker_thread_(rtc::Thread::Current()),
+    : worker_thread_(call->worker_thread()),
       ssrcs_(sp.ssrcs),
       ssrc_groups_(sp.ssrc_groups),
       call_(call),

media/engine/webrtc_video_engine.h

@@ -43,10 +43,6 @@ class VideoEncoderFactory;
 struct MediaConfig;
 }  // namespace webrtc
 
-namespace rtc {
-class Thread;
-}  // namespace rtc
-
 namespace cricket {
 
 class WebRtcVideoChannel;
@@ -402,7 +398,7 @@ class WebRtcVideoChannel : public VideoMediaChannel,
         RTC_EXCLUSIVE_LOCKS_REQUIRED(&thread_checker_);
 
     webrtc::SequenceChecker thread_checker_;
-    rtc::Thread* worker_thread_;
+    webrtc::TaskQueueBase* const worker_thread_;
     const std::vector<uint32_t> ssrcs_ RTC_GUARDED_BY(&thread_checker_);
     const std::vector<SsrcGroup> ssrc_groups_ RTC_GUARDED_BY(&thread_checker_);
     webrtc::Call* const call_;
@@ -553,7 +549,7 @@ class WebRtcVideoChannel : public VideoMediaChannel,
   void FillSendAndReceiveCodecStats(VideoMediaInfo* video_media_info)
       RTC_EXCLUSIVE_LOCKS_REQUIRED(thread_checker_);
 
-  rtc::Thread* const worker_thread_;
+  webrtc::TaskQueueBase* const worker_thread_;
   webrtc::ScopedTaskSafety task_safety_;
   webrtc::SequenceChecker network_thread_checker_;
   webrtc::SequenceChecker thread_checker_;

media/engine/webrtc_voice_engine.cc

@@ -50,7 +50,6 @@
 #include "rtc_base/task_utils/pending_task_safety_flag.h"
 #include "rtc_base/task_utils/to_queued_task.h"
 #include "rtc_base/third_party/base64/base64.h"
-#include "rtc_base/thread.h"
 #include "rtc_base/trace_event.h"
 #include "system_wrappers/include/metrics.h"
 
@@ -377,7 +376,7 @@ VoiceMediaChannel* WebRtcVoiceEngine::CreateMediaChannel(
     const MediaConfig& config,
     const AudioOptions& options,
     const webrtc::CryptoOptions& crypto_options) {
-  RTC_DCHECK_RUN_ON(&worker_thread_checker_);
+  RTC_DCHECK_RUN_ON(call->worker_thread());
   return new WebRtcVoiceMediaChannel(this, config, options, crypto_options,
                                      call);
 }
@@ -626,19 +625,6 @@ WebRtcVoiceEngine::GetRtpHeaderExtensions() const {
   return result;
 }
 
-void WebRtcVoiceEngine::RegisterChannel(WebRtcVoiceMediaChannel* channel) {
-  RTC_DCHECK_RUN_ON(&worker_thread_checker_);
-  RTC_DCHECK(channel);
-  channels_.push_back(channel);
-}
-
-void WebRtcVoiceEngine::UnregisterChannel(WebRtcVoiceMediaChannel* channel) {
-  RTC_DCHECK_RUN_ON(&worker_thread_checker_);
-  auto it = absl::c_find(channels_, channel);
-  RTC_DCHECK(it != channels_.end());
-  channels_.erase(it);
-}
-
 bool WebRtcVoiceEngine::StartAecDump(webrtc::FileWrapper file,
                                      int64_t max_size_bytes) {
   RTC_DCHECK_RUN_ON(&worker_thread_checker_);
@@ -1396,18 +1382,16 @@ WebRtcVoiceMediaChannel::WebRtcVoiceMediaChannel(
     const webrtc::CryptoOptions& crypto_options,
     webrtc::Call* call)
     : VoiceMediaChannel(config),
-      worker_thread_(rtc::Thread::Current()),
+      worker_thread_(call->worker_thread()),
       engine_(engine),
       call_(call),
       audio_config_(config.audio),
       crypto_options_(crypto_options),
       audio_red_for_opus_trial_enabled_(
           IsEnabled(call->trials(), "WebRTC-Audio-Red-For-Opus")) {
-  RTC_DCHECK_RUN_ON(worker_thread_);
   network_thread_checker_.Detach();
   RTC_LOG(LS_VERBOSE) << "WebRtcVoiceMediaChannel::WebRtcVoiceMediaChannel";
   RTC_DCHECK(call);
-  engine->RegisterChannel(this);
   SetOptions(options);
 }
 
@@ -1423,7 +1407,6 @@ WebRtcVoiceMediaChannel::~WebRtcVoiceMediaChannel() {
   while (!recv_streams_.empty()) {
     RemoveRecvStream(recv_streams_.begin()->first);
   }
-  engine()->UnregisterChannel(this);
 }
 
 bool WebRtcVoiceMediaChannel::SetSendParameters(

media/engine/webrtc_voice_engine.h

@@ -80,12 +80,6 @@ class WebRtcVoiceEngine final : public VoiceEngineInterface {
   std::vector<webrtc::RtpHeaderExtensionCapability> GetRtpHeaderExtensions()
       const override;
 
-  // For tracking WebRtc channels. Needed because we have to pause them
-  // all when switching devices.
-  // May only be called by WebRtcVoiceMediaChannel.
-  void RegisterChannel(WebRtcVoiceMediaChannel* channel);
-  void UnregisterChannel(WebRtcVoiceMediaChannel* channel);
-
   // Starts AEC dump using an existing file. A maximum file size in bytes can be
   // specified. When the maximum file size is reached, logging is stopped and
   // the file is closed. If max_size_bytes is set to <= 0, no limit will be
@@ -129,7 +123,6 @@ class WebRtcVoiceEngine final : public VoiceEngineInterface {
   rtc::scoped_refptr<webrtc::AudioState> audio_state_;
   std::vector<AudioCodec> send_codecs_;
   std::vector<AudioCodec> recv_codecs_;
-  std::vector<WebRtcVoiceMediaChannel*> channels_;
   bool is_dumping_aec_ = false;
   bool initialized_ = false;
 

pc/channel.cc

@@ -295,7 +295,11 @@ void BaseChannel::Enable(bool enable) {
 bool BaseChannel::SetLocalContent(const MediaContentDescription* content,
                                   SdpType type,
                                   std::string* error_desc) {
+  RTC_DCHECK_RUN_ON(signaling_thread());
   TRACE_EVENT0("webrtc", "BaseChannel::SetLocalContent");
+
+  SetContent_s(content, type);
+
   return InvokeOnWorker<bool>(RTC_FROM_HERE, [this, content, type, error_desc] {
     RTC_DCHECK_RUN_ON(worker_thread());
     return SetLocalContent_w(content, type, error_desc);
@@ -305,13 +309,24 @@ bool BaseChannel::SetLocalContent(const MediaContentDescription* content,
 bool BaseChannel::SetRemoteContent(const MediaContentDescription* content,
                                    SdpType type,
                                    std::string* error_desc) {
+  RTC_DCHECK_RUN_ON(signaling_thread());
   TRACE_EVENT0("webrtc", "BaseChannel::SetRemoteContent");
+
+  SetContent_s(content, type);
+
   return InvokeOnWorker<bool>(RTC_FROM_HERE, [this, content, type, error_desc] {
     RTC_DCHECK_RUN_ON(worker_thread());
     return SetRemoteContent_w(content, type, error_desc);
   });
 }
 
+void BaseChannel::SetContent_s(const MediaContentDescription* content,
+                               SdpType type) {
+  RTC_DCHECK(content);
+  if (type == SdpType::kAnswer)
+    negotiated_header_extensions_ = content->rtp_header_extensions();
+}
+
 bool BaseChannel::SetPayloadTypeDemuxingEnabled(bool enabled) {
   TRACE_EVENT0("webrtc", "BaseChannel::SetPayloadTypeDemuxingEnabled");
   return InvokeOnWorker<bool>(RTC_FROM_HERE, [this, enabled] {
@@ -853,16 +868,8 @@ void BaseChannel::SignalSentPacket_n(const rtc::SentPacket& sent_packet) {
   media_channel()->OnPacketSent(sent_packet);
 }
 
-void BaseChannel::SetNegotiatedHeaderExtensions_w(
-    const RtpHeaderExtensions& extensions) {
-  TRACE_EVENT0("webrtc", __func__);
-  webrtc::MutexLock lock(&negotiated_header_extensions_lock_);
-  negotiated_header_extensions_ = extensions;
-}
-
 RtpHeaderExtensions BaseChannel::GetNegotiatedRtpHeaderExtensions() const {
   RTC_DCHECK_RUN_ON(signaling_thread());
-  webrtc::MutexLock lock(&negotiated_header_extensions_lock_);
   return negotiated_header_extensions_;
 }
 
@@ -913,26 +920,19 @@ bool VoiceChannel::SetLocalContent_w(const MediaContentDescription* content,
   RTC_DCHECK_RUN_ON(worker_thread());
   RTC_LOG(LS_INFO) << "Setting local voice description for " << ToString();
 
-  RTC_DCHECK(content);
-  if (!content) {
-    SafeSetError("Can't find audio content in local description.", error_desc);
-    return false;
-  }
-
-  const AudioContentDescription* audio = content->as_audio();
-
-  if (type == SdpType::kAnswer)
-    SetNegotiatedHeaderExtensions_w(audio->rtp_header_extensions());
-
   RtpHeaderExtensions rtp_header_extensions =
-      GetFilteredRtpHeaderExtensions(audio->rtp_header_extensions());
+      GetFilteredRtpHeaderExtensions(content->rtp_header_extensions());
+  // TODO(tommi): There's a hop to the network thread here.
+  // some of the below is also network thread related.
   UpdateRtpHeaderExtensionMap(rtp_header_extensions);
-  media_channel()->SetExtmapAllowMixed(audio->extmap_allow_mixed());
+  media_channel()->SetExtmapAllowMixed(content->extmap_allow_mixed());
 
   AudioRecvParameters recv_params = last_recv_params_;
   RtpParametersFromMediaDescription(
-      audio, rtp_header_extensions,
+      content->as_audio(), rtp_header_extensions,
-      webrtc::RtpTransceiverDirectionHasRecv(audio->direction()), &recv_params);
+      webrtc::RtpTransceiverDirectionHasRecv(content->direction()),
+      &recv_params);
+
   if (!media_channel()->SetRecvParameters(recv_params)) {
     SafeSetError(
         "Failed to set local audio description recv parameters for m-section "
@@ -942,8 +942,8 @@ bool VoiceChannel::SetLocalContent_w(const MediaContentDescription* content,
     return false;
   }
 
-  if (webrtc::RtpTransceiverDirectionHasRecv(audio->direction())) {
+  if (webrtc::RtpTransceiverDirectionHasRecv(content->direction())) {
-    for (const AudioCodec& codec : audio->codecs()) {
+    for (const AudioCodec& codec : content->as_audio()->codecs()) {
       MaybeAddHandledPayloadType(codec.id);
     }
     // Need to re-register the sink to update the handled payload.
@@ -959,7 +959,7 @@ bool VoiceChannel::SetLocalContent_w(const MediaContentDescription* content,
   // only give it to the media channel once we have a remote
   // description too (without a remote description, we won't be able
   // to send them anyway).
-  if (!UpdateLocalStreams_w(audio->streams(), type, error_desc)) {
+  if (!UpdateLocalStreams_w(content->as_audio()->streams(), type, error_desc)) {
     SafeSetError(
         "Failed to set local audio description streams for m-section with "
         "mid='" +
@@ -980,17 +980,8 @@ bool VoiceChannel::SetRemoteContent_w(const MediaContentDescription* content,
   RTC_DCHECK_RUN_ON(worker_thread());
   RTC_LOG(LS_INFO) << "Setting remote voice description for " << ToString();
 
-  RTC_DCHECK(content);
-  if (!content) {
-    SafeSetError("Can't find audio content in remote description.", error_desc);
-    return false;
-  }
-
   const AudioContentDescription* audio = content->as_audio();
 
-  if (type == SdpType::kAnswer)
-    SetNegotiatedHeaderExtensions_w(audio->rtp_header_extensions());
-
   RtpHeaderExtensions rtp_header_extensions =
       GetFilteredRtpHeaderExtensions(audio->rtp_header_extensions());
 
@@ -1091,26 +1082,17 @@ bool VideoChannel::SetLocalContent_w(const MediaContentDescription* content,
   RTC_DCHECK_RUN_ON(worker_thread());
   RTC_LOG(LS_INFO) << "Setting local video description for " << ToString();
 
-  RTC_DCHECK(content);
-  if (!content) {
-    SafeSetError("Can't find video content in local description.", error_desc);
-    return false;
-  }
-
-  const VideoContentDescription* video = content->as_video();
-
-  if (type == SdpType::kAnswer)
-    SetNegotiatedHeaderExtensions_w(video->rtp_header_extensions());
-
   RtpHeaderExtensions rtp_header_extensions =
-      GetFilteredRtpHeaderExtensions(video->rtp_header_extensions());
+      GetFilteredRtpHeaderExtensions(content->rtp_header_extensions());
   UpdateRtpHeaderExtensionMap(rtp_header_extensions);
-  media_channel()->SetExtmapAllowMixed(video->extmap_allow_mixed());
+  media_channel()->SetExtmapAllowMixed(content->extmap_allow_mixed());
 
   VideoRecvParameters recv_params = last_recv_params_;
+
   RtpParametersFromMediaDescription(
-      video, rtp_header_extensions,
+      content->as_video(), rtp_header_extensions,
-      webrtc::RtpTransceiverDirectionHasRecv(video->direction()), &recv_params);
+      webrtc::RtpTransceiverDirectionHasRecv(content->direction()),
+      &recv_params);
 
   VideoSendParameters send_params = last_send_params_;
 
@@ -1143,8 +1125,8 @@ bool VideoChannel::SetLocalContent_w(const MediaContentDescription* content,
     return false;
   }
 
-  if (webrtc::RtpTransceiverDirectionHasRecv(video->direction())) {
+  if (webrtc::RtpTransceiverDirectionHasRecv(content->direction())) {
-    for (const VideoCodec& codec : video->codecs()) {
+    for (const VideoCodec& codec : content->as_video()->codecs()) {
       MaybeAddHandledPayloadType(codec.id);
     }
     // Need to re-register the sink to update the handled payload.
@@ -1170,7 +1152,7 @@ bool VideoChannel::SetLocalContent_w(const MediaContentDescription* content,
   // only give it to the media channel once we have a remote
   // description too (without a remote description, we won't be able
   // to send them anyway).
-  if (!UpdateLocalStreams_w(video->streams(), type, error_desc)) {
+  if (!UpdateLocalStreams_w(content->as_video()->streams(), type, error_desc)) {
     SafeSetError(
         "Failed to set local video description streams for m-section with "
         "mid='" +
@@ -1191,17 +1173,8 @@ bool VideoChannel::SetRemoteContent_w(const MediaContentDescription* content,
   RTC_DCHECK_RUN_ON(worker_thread());
   RTC_LOG(LS_INFO) << "Setting remote video description for " << ToString();
 
-  RTC_DCHECK(content);
-  if (!content) {
-    SafeSetError("Can't find video content in remote description.", error_desc);
-    return false;
-  }
-
   const VideoContentDescription* video = content->as_video();
 
-  if (type == SdpType::kAnswer)
-    SetNegotiatedHeaderExtensions_w(video->rtp_header_extensions());
-
   RtpHeaderExtensions rtp_header_extensions =
       GetFilteredRtpHeaderExtensions(video->rtp_header_extensions());
 

pc/channel.h

@@ -59,7 +59,6 @@
 #include "rtc_base/network/sent_packet.h"
 #include "rtc_base/network_route.h"
 #include "rtc_base/socket.h"
-#include "rtc_base/synchronization/mutex.h"
 #include "rtc_base/task_utils/pending_task_safety_flag.h"
 #include "rtc_base/third_party/sigslot/sigslot.h"
 #include "rtc_base/thread.h"
@@ -306,9 +305,6 @@ class BaseChannel : public ChannelInterface,
   // Return description of media channel to facilitate logging
   std::string ToString() const;
 
-  void SetNegotiatedHeaderExtensions_w(const RtpHeaderExtensions& extensions)
-      RTC_RUN_ON(worker_thread());
-
   // ChannelInterface overrides
   RtpHeaderExtensions GetNegotiatedRtpHeaderExtensions() const override;
 
@@ -316,6 +312,8 @@ class BaseChannel : public ChannelInterface,
   bool ConnectToRtpTransport() RTC_RUN_ON(network_thread());
   void DisconnectFromRtpTransport() RTC_RUN_ON(network_thread());
   void SignalSentPacket_n(const rtc::SentPacket& sent_packet);
+  void SetContent_s(const MediaContentDescription* content,
+                    webrtc::SdpType type) RTC_RUN_ON(signaling_thread());
 
   rtc::Thread* const worker_thread_;
   rtc::Thread* const network_thread_;
@@ -382,13 +380,11 @@ class BaseChannel : public ChannelInterface,
   // This object is not owned by the channel so it must outlive it.
   rtc::UniqueRandomIdGenerator* const ssrc_generator_;
 
-  // |negotiated_header_extensions_| is read on the signaling thread, but
+  // |negotiated_header_extensions_| is read and written to on the signaling
-  // written on the worker thread while being sync-invoked from the signal
+  // thread from the SdpOfferAnswerHandler class (e.g.
-  // thread in SdpOfferAnswerHandler::PushdownMediaDescription(). Hence the lock
+  // PushdownMediaDescription().
-  // isn't strictly needed, but it's anyway placed here for future safeness.
-  mutable webrtc::Mutex negotiated_header_extensions_lock_;
   RtpHeaderExtensions negotiated_header_extensions_
-      RTC_GUARDED_BY(negotiated_header_extensions_lock_);
+      RTC_GUARDED_BY(signaling_thread());
 };
 
 // VoiceChannel is a specialization that adds support for early media, DTMF,