Code consolidation in BaseChannel and derived classes.

This is a bit of refactoring to clear the way for some more upcoming
changes and fix little oddities here and there that are basically
artifacts of many small incremental changes throughout the years.

* Remove the CryptoOptions member variable and instead only keep around
  the filter for rtp header extensions.
* Remove several member methods that only forwarded calls to
  media_channel() and effectively reduced readability.
* Consolidated quite a bit of code related to UpdateRemoteStreams_w
  and the copy/pasted code in the Video/Voice classes around calling it.
* UpdateRemoteStreams_w now returns an error when it's encountered.
  Before, an error would still be returned in those cases but all
  operations were unnecessarily performed.

Bug: none
Change-Id: I85a37b9e8f00584aa794abef11abfe89dec5d0a6
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/244098
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35637}

pc/channel.cc

@@ -100,9 +100,13 @@ void RtpParametersFromMediaDescription(
 template <class Codec>
 void RtpSendParametersFromMediaDescription(
     const MediaContentDescriptionImpl<Codec>* desc,
-    const RtpHeaderExtensions& extensions,
+    webrtc::RtpExtension::Filter extensions_filter,
-    bool is_stream_active,
     RtpSendParameters<Codec>* send_params) {
+  RtpHeaderExtensions extensions =
+      webrtc::RtpExtension::DeduplicateHeaderExtensions(
+          desc->rtp_header_extensions(), extensions_filter);
+  const bool is_stream_active =
+      webrtc::RtpTransceiverDirectionHasRecv(desc->direction());
   RtpParametersFromMediaDescription(desc, extensions, is_stream_active,
                                     send_params);
   send_params->max_bandwidth_bps = desc->bandwidth();
@@ -122,7 +126,10 @@ BaseChannel::BaseChannel(rtc::Thread* worker_thread,
       signaling_thread_(signaling_thread),
       alive_(PendingTaskSafetyFlag::Create()),
       srtp_required_(srtp_required),
-      crypto_options_(crypto_options),
+      extensions_filter_(
+          crypto_options.srtp.enable_encrypted_rtp_header_extensions
+              ? webrtc::RtpExtension::kPreferEncryptedExtension
+              : webrtc::RtpExtension::kDiscardEncryptedExtension),
       media_channel_(std::move(media_channel)),
       demuxer_criteria_(content_name),
       ssrc_generator_(ssrc_generator) {
@@ -539,18 +546,6 @@ void BaseChannel::ChannelNotWritable_n() {
   RTC_LOG(LS_INFO) << "Channel not writable (" << ToString() << ")";
 }
 
-bool BaseChannel::AddRecvStream_w(const StreamParams& sp) {
-  return media_channel()->AddRecvStream(sp);
-}
-
-bool BaseChannel::RemoveRecvStream_w(uint32_t ssrc) {
-  return media_channel()->RemoveRecvStream(ssrc);
-}
-
-void BaseChannel::ResetUnsignaledRecvStream_w() {
-  media_channel()->ResetUnsignaledRecvStream();
-}
-
 bool BaseChannel::SetPayloadTypeDemuxingEnabled_w(bool enabled) {
   if (enabled == payload_type_demuxing_enabled_) {
     return true;
@@ -653,22 +648,35 @@ bool BaseChannel::UpdateLocalStreams_w(const std::vector<StreamParams>& streams,
   return ret;
 }
 
-bool BaseChannel::UpdateRemoteStreams_w(
+bool BaseChannel::UpdateRemoteStreams_w(const MediaContentDescription* content,
-    const std::vector<StreamParams>& streams,
+                                        SdpType type,
-    SdpType type,
+                                        std::string& error_desc) {
-    std::string& error_desc) {
+  RTC_LOG_THREAD_BLOCK_COUNT();
+  bool needs_re_registration = false;
+  if (!webrtc::RtpTransceiverDirectionHasSend(content->direction())) {
+    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_;
+    }
+  }
+
+  const std::vector<StreamParams>& streams = content->streams();
+  const bool new_has_unsignaled_ssrcs = HasStreamWithNoSsrcs(streams);
+  const bool old_has_unsignaled_ssrcs = HasStreamWithNoSsrcs(remote_streams_);
+
   // Check for streams that have been removed.
-  bool ret = true;
   for (const StreamParams& old_stream : remote_streams_) {
     // If we no longer have an unsignaled stream, we would like to remove
     // the unsignaled stream params that are cached.
-    if (!old_stream.has_ssrcs() && !HasStreamWithNoSsrcs(streams)) {
+    if (!old_stream.has_ssrcs() && !new_has_unsignaled_ssrcs) {
-      ResetUnsignaledRecvStream_w();
+      media_channel()->ResetUnsignaledRecvStream();
       RTC_LOG(LS_INFO) << "Reset unsignaled remote stream for " << ToString()
                        << ".";
     } else if (old_stream.has_ssrcs() &&
                !GetStreamBySsrc(streams, old_stream.first_ssrc())) {
-      if (RemoveRecvStream_w(old_stream.first_ssrc())) {
+      if (media_channel()->RemoveRecvStream(old_stream.first_ssrc())) {
         RTC_LOG(LS_INFO) << "Remove remote ssrc: " << old_stream.first_ssrc()
                          << " from " << ToString() << ".";
       } else {
@@ -676,19 +684,20 @@ bool BaseChannel::UpdateRemoteStreams_w(
             "Failed to remove remote stream with ssrc %u from m-section with "
             "mid='%s'.",
             old_stream.first_ssrc(), content_name().c_str());
-        ret = false;
+        return false;
       }
     }
   }
-  demuxer_criteria_.ssrcs().clear();
+
   // Check for new streams.
+  webrtc::flat_set<uint32_t> ssrcs;
   for (const StreamParams& new_stream : streams) {
     // We allow a StreamParams with an empty list of SSRCs, in which case the
     // MediaChannel will cache the parameters and use them for any unsignaled
     // stream received later.
-    if ((!new_stream.has_ssrcs() && !HasStreamWithNoSsrcs(remote_streams_)) ||
+    if ((!new_stream.has_ssrcs() && !old_has_unsignaled_ssrcs) ||
         !GetStreamBySsrc(remote_streams_, new_stream.first_ssrc())) {
-      if (AddRecvStream_w(new_stream)) {
+      if (media_channel()->AddRecvStream(new_stream)) {
         RTC_LOG(LS_INFO) << "Add remote ssrc: "
                          << (new_stream.has_ssrcs()
                                  ? std::to_string(new_stream.first_ssrc())
@@ -701,28 +710,41 @@ bool BaseChannel::UpdateRemoteStreams_w(
                              ? std::to_string(new_stream.first_ssrc()).c_str()
                              : "unsignaled",
                          ToString().c_str());
-        ret = false;
+        return false;
       }
     }
     // Update the receiving SSRCs.
-    demuxer_criteria_.ssrcs().insert(new_stream.ssrcs.begin(),
+    ssrcs.insert(new_stream.ssrcs.begin(), new_stream.ssrcs.end());
-                                     new_stream.ssrcs.end());
   }
-  // Re-register the sink to update the receiving ssrcs.
+
-  if (!RegisterRtpDemuxerSink_w()) {
+  if (demuxer_criteria_.ssrcs() != ssrcs) {
-    RTC_LOG(LS_ERROR) << "Failed to set up demuxing for " << ToString();
+    demuxer_criteria_.ssrcs() = std::move(ssrcs);
-    ret = false;
+    needs_re_registration = true;
   }
+
+  RTC_DCHECK_BLOCK_COUNT_NO_MORE_THAN(0);
+
+  // Re-register the sink to update after changing the demuxer criteria.
+  if (needs_re_registration && !RegisterRtpDemuxerSink_w()) {
+    error_desc = StringFormat("Failed to set up audio demuxing for mid='%s'.",
+                              content_name().c_str());
+    return false;
+  }
+
   remote_streams_ = streams;
-  return ret;
+
+  set_remote_content_direction(content->direction());
+  UpdateMediaSendRecvState_w();
+
+  RTC_DCHECK_BLOCK_COUNT_NO_MORE_THAN(1);
+
+  return true;
 }
 
 RtpHeaderExtensions BaseChannel::GetDeduplicatedRtpHeaderExtensions(
     const RtpHeaderExtensions& extensions) {
-  return webrtc::RtpExtension::DeduplicateHeaderExtensions(
+  return webrtc::RtpExtension::DeduplicateHeaderExtensions(extensions,
-      extensions, crypto_options_.srtp.enable_encrypted_rtp_header_extensions
+                                                           extensions_filter_);
-                      ? webrtc::RtpExtension::kPreferEncryptedExtension
-                      : webrtc::RtpExtension::kDiscardEncryptedExtension);
 }
 
 void BaseChannel::MaybeAddHandledPayloadType(int payload_type) {
@@ -735,9 +757,11 @@ void BaseChannel::MaybeAddHandledPayloadType(int payload_type) {
   payload_types_.insert(static_cast<uint8_t>(payload_type));
 }
 
-void BaseChannel::ClearHandledPayloadTypes() {
+bool BaseChannel::ClearHandledPayloadTypes() {
+  const bool was_empty = demuxer_criteria_.payload_types().empty();
   demuxer_criteria_.payload_types().clear();
   payload_types_.clear();
+  return !was_empty;
 }
 
 void BaseChannel::SignalSentPacket_n(const rtc::SentPacket& sent_packet) {
@@ -818,7 +842,6 @@ bool VoiceChannel::SetLocalContent_w(const MediaContentDescription* content,
     }
     // Need to re-register the sink to update the handled payload.
     if (!RegisterRtpDemuxerSink_w()) {
-      RTC_LOG(LS_ERROR) << "Failed to set up audio demuxing for " << ToString();
       error_desc = StringFormat("Failed to set up audio demuxing for mid='%s'.",
                                 content_name().c_str());
       return false;
@@ -847,15 +870,9 @@ bool VoiceChannel::SetRemoteContent_w(const MediaContentDescription* content,
   TRACE_EVENT0("webrtc", "VoiceChannel::SetRemoteContent_w");
   RTC_LOG(LS_INFO) << "Setting remote voice description for " << ToString();
 
-  const AudioContentDescription* audio = content->as_audio();
-
-  RtpHeaderExtensions rtp_header_extensions =
-      GetDeduplicatedRtpHeaderExtensions(audio->rtp_header_extensions());
-
   AudioSendParameters send_params = last_send_params_;
-  RtpSendParametersFromMediaDescription(
+  RtpSendParametersFromMediaDescription(content->as_audio(),
-      audio, rtp_header_extensions,
+                                        extensions_filter(), &send_params);
-      webrtc::RtpTransceiverDirectionHasRecv(audio->direction()), &send_params);
   send_params.mid = content_name();
 
   bool parameters_applied = media_channel()->SetSendParameters(send_params);
@@ -868,32 +885,7 @@ bool VoiceChannel::SetRemoteContent_w(const MediaContentDescription* content,
   }
   last_send_params_ = send_params;
 
-  if (!webrtc::RtpTransceiverDirectionHasSend(content->direction())) {
+  return UpdateRemoteStreams_w(content, type, error_desc);
-    RTC_DLOG(LS_VERBOSE) << "SetRemoteContent_w: remote side will not send - "
-                            "disable payload type demuxing for "
-                         << ToString();
-    ClearHandledPayloadTypes();
-    if (!RegisterRtpDemuxerSink_w()) {
-      RTC_LOG(LS_ERROR) << "Failed to update audio demuxing for " << ToString();
-      return false;
-    }
-  }
-
-  // TODO(pthatcher): Move remote streams into AudioRecvParameters,
-  // and only give it to the media channel once we have a local
-  // description too (without a local description, we won't be able to
-  // recv them anyway).
-  if (!UpdateRemoteStreams_w(audio->streams(), type, error_desc)) {
-    error_desc = StringFormat(
-        "Failed to set remote audio description streams for m-section with "
-        "mid='%s'.",
-        content_name().c_str());
-    return false;
-  }
-
-  set_remote_content_direction(content->direction());
-  UpdateMediaSendRecvState_w();
-  return true;
 }
 
 VideoChannel::VideoChannel(rtc::Thread* worker_thread,
@@ -924,11 +916,7 @@ 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.
   bool send = IsReadyToSendMedia_w();
-  if (!media_channel()->SetSend(send)) {
+  media_channel()->SetSend(send);
-    RTC_LOG(LS_ERROR) << "Failed to SetSend on video channel: " + ToString();
-    // TODO(gangji): Report error back to server.
-  }
-
   RTC_LOG(LS_INFO) << "Changing video state, send=" << send << " for "
                    << ToString();
 }
@@ -992,7 +980,6 @@ bool VideoChannel::SetLocalContent_w(const MediaContentDescription* content,
     }
     // Need to re-register the sink to update the handled payload.
     if (!RegisterRtpDemuxerSink_w()) {
-      RTC_LOG(LS_ERROR) << "Failed to set up video demuxing for " << ToString();
       error_desc = StringFormat("Failed to set up video demuxing for mid='%s'.",
                                 content_name().c_str());
       return false;
@@ -1033,17 +1020,11 @@ bool VideoChannel::SetRemoteContent_w(const MediaContentDescription* content,
 
   const VideoContentDescription* video = content->as_video();
 
-  RtpHeaderExtensions rtp_header_extensions =
-      GetDeduplicatedRtpHeaderExtensions(video->rtp_header_extensions());
-
   VideoSendParameters send_params = last_send_params_;
-  RtpSendParametersFromMediaDescription(
+  RtpSendParametersFromMediaDescription(video, extensions_filter(),
-      video, rtp_header_extensions,
+                                        &send_params);
-      webrtc::RtpTransceiverDirectionHasRecv(video->direction()), &send_params);
-  if (video->conference_mode()) {
-    send_params.conference_mode = true;
-  }
   send_params.mid = content_name();
+  send_params.conference_mode = video->conference_mode();
 
   VideoRecvParameters recv_params = last_recv_params_;
 
@@ -1085,31 +1066,7 @@ bool VideoChannel::SetRemoteContent_w(const MediaContentDescription* content,
     last_recv_params_ = recv_params;
   }
 
-  if (!webrtc::RtpTransceiverDirectionHasSend(content->direction())) {
+  return UpdateRemoteStreams_w(content, type, error_desc);
-    RTC_DLOG(LS_VERBOSE) << "SetRemoteContent_w: remote side will not send - "
-                            "disable payload type demuxing for "
-                         << ToString();
-    ClearHandledPayloadTypes();
-    if (!RegisterRtpDemuxerSink_w()) {
-      RTC_LOG(LS_ERROR) << "Failed to update video demuxing for " << ToString();
-      return false;
-    }
-  }
-
-  // TODO(pthatcher): Move remote streams into VideoRecvParameters,
-  // and only give it to the media channel once we have a local
-  // description too (without a local description, we won't be able to
-  // recv them anyway).
-  if (!UpdateRemoteStreams_w(video->streams(), type, error_desc)) {
-    error_desc = StringFormat(
-        "Failed to set remote video description streams for m-section with "
-        "mid='%s'.",
-        content_name().c_str());
-    return false;
-  }
-  set_remote_content_direction(content->direction());
-  UpdateMediaSendRecvState_w();
-  return true;
 }
 
 }  // namespace cricket

pc/channel.h

@@ -213,6 +213,10 @@ class BaseChannel : public ChannelInterface,
     return remote_content_direction_;
   }
 
+  webrtc::RtpExtension::Filter extensions_filter() const {
+    return extensions_filter_;
+  }
+
   bool enabled() const RTC_RUN_ON(worker_thread()) { return enabled_; }
   rtc::Thread* signaling_thread() const { return signaling_thread_; }
 
@@ -252,13 +256,8 @@ class BaseChannel : public ChannelInterface,
   void ChannelWritable_n() RTC_RUN_ON(network_thread());
   void ChannelNotWritable_n() RTC_RUN_ON(network_thread());
 
-  bool AddRecvStream_w(const StreamParams& sp) RTC_RUN_ON(worker_thread());
-  bool RemoveRecvStream_w(uint32_t ssrc) RTC_RUN_ON(worker_thread());
-  void ResetUnsignaledRecvStream_w() RTC_RUN_ON(worker_thread());
   bool SetPayloadTypeDemuxingEnabled_w(bool enabled)
       RTC_RUN_ON(worker_thread());
-  bool AddSendStream_w(const StreamParams& sp) RTC_RUN_ON(worker_thread());
-  bool RemoveSendStream_w(uint32_t ssrc) RTC_RUN_ON(worker_thread());
 
   // Should be called whenever the conditions for
   // IsReadyToReceiveMedia/IsReadyToSendMedia are satisfied (or unsatisfied).
@@ -269,7 +268,7 @@ class BaseChannel : public ChannelInterface,
                             webrtc::SdpType type,
                             std::string& error_desc)
       RTC_RUN_ON(worker_thread());
-  bool UpdateRemoteStreams_w(const std::vector<StreamParams>& streams,
+  bool UpdateRemoteStreams_w(const MediaContentDescription* content,
                              webrtc::SdpType type,
                              std::string& error_desc)
       RTC_RUN_ON(worker_thread());
@@ -292,7 +291,9 @@ class BaseChannel : public ChannelInterface,
   // enabled.
   void MaybeAddHandledPayloadType(int payload_type) RTC_RUN_ON(worker_thread());
 
-  void ClearHandledPayloadTypes() RTC_RUN_ON(worker_thread());
+  // Returns true iff the demuxer payload type criteria was non-empty before
+  // clearing.
+  bool ClearHandledPayloadTypes() RTC_RUN_ON(worker_thread());
 
   void UpdateRtpHeaderExtensionMap(
       const RtpHeaderExtensions& header_extensions);
@@ -327,24 +328,9 @@ class BaseChannel : public ChannelInterface,
   bool was_ever_writable_ RTC_GUARDED_BY(worker_thread()) = false;
   const bool srtp_required_ = true;
 
-  // TODO(tommi): This field shouldn't be necessary. It's a copy of
+  // Set to either kPreferEncryptedExtension or kDiscardEncryptedExtension
-  // PeerConnection::GetCryptoOptions(), which is const state. It's also only
+  // based on the supplied CryptoOptions.
-  // used to filter header extensions when calling
+  const webrtc::RtpExtension::Filter extensions_filter_;
-  // `rtp_transport_->UpdateRtpHeaderExtensionMap()` when the local/remote
-  // content description is updated. Since the transport is actually owned
-  // by the transport controller that also gets updated whenever the content
-  // description changes, it seems we have two paths into the transports, along
-  // with several thread hops via various classes (such as the Channel classes)
-  // that only serve as additional layers and store duplicate state. The Jsep*
-  // family of classes already apply session description updates on the network
-  // thread every time it changes.
-  // For the Channel classes, we should be able to get rid of:
-  // * crypto_options (and fewer construction parameters)_
-  // * UpdateRtpHeaderExtensionMap
-  // * GetFilteredRtpHeaderExtensions
-  // * Blocking thread hop to the network thread for every call to set
-  //   local/remote content is updated.
-  const webrtc::CryptoOptions crypto_options_;
 
   // MediaChannel related members that should be accessed from the worker
   // thread.