Move header negotiation state to transceivers.

The channel classes have stored the negotiated headers but a more
natural place to store them is in the RtpTransceiver class where
RtpHeaderExtension state is managed as well as the implementation of
the only method that depends on the stored state,
HeaderExtensionsNegotiated().

Also adding a TODO for further improvements where we're unnecessarily
storing state in the channel classes for the purposes of the transports.

Bug: webrtc:12726
Change-Id: If36668e3e49782ddeada23ebed126ee2c4935b8c
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/216691
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Tommi <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33917}

pc/channel.cc

@@ -293,36 +293,17 @@ void BaseChannel::Enable(bool enable) {
 bool BaseChannel::SetLocalContent(const MediaContentDescription* content,
                                   SdpType type,
                                   std::string* error_desc) {
-  RTC_DCHECK_RUN_ON(signaling_thread());
+  RTC_DCHECK_RUN_ON(worker_thread());
   TRACE_EVENT0("webrtc", "BaseChannel::SetLocalContent");
-
+  return SetLocalContent_w(content, type, error_desc);
-  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);
-  });
 }
 
 bool BaseChannel::SetRemoteContent(const MediaContentDescription* content,
                                    SdpType type,
                                    std::string* error_desc) {
-  RTC_DCHECK_RUN_ON(signaling_thread());
+  RTC_DCHECK_RUN_ON(worker_thread());
   TRACE_EVENT0("webrtc", "BaseChannel::SetRemoteContent");
-
+  return SetRemoteContent_w(content, type, error_desc);
-  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) {
@@ -863,11 +844,6 @@ void BaseChannel::SignalSentPacket_n(const rtc::SentPacket& sent_packet) {
   media_channel()->OnPacketSent(sent_packet);
 }
 
-RtpHeaderExtensions BaseChannel::GetNegotiatedRtpHeaderExtensions() const {
-  RTC_DCHECK_RUN_ON(signaling_thread());
-  return negotiated_header_extensions_;
-}
-
 VoiceChannel::VoiceChannel(rtc::Thread* worker_thread,
                            rtc::Thread* network_thread,
                            rtc::Thread* signaling_thread,

pc/channel.h

@@ -288,13 +288,6 @@ class BaseChannel : public ChannelInterface,
   // From MessageHandler
   void OnMessage(rtc::Message* pmsg) override;
 
-  // Helper function template for invoking methods on the worker thread.
-  template <class T>
-  T InvokeOnWorker(const rtc::Location& posted_from,
-                   rtc::FunctionView<T()> functor) {
-    return worker_thread_->Invoke<T>(posted_from, functor);
-  }
-
   // Add |payload_type| to |demuxer_criteria_| if payload type demuxing is
   // enabled.
   void MaybeAddHandledPayloadType(int payload_type) RTC_RUN_ON(worker_thread());
@@ -309,15 +302,10 @@ class BaseChannel : public ChannelInterface,
   // Return description of media channel to facilitate logging
   std::string ToString() const;
 
-  // ChannelInterface overrides
-  RtpHeaderExtensions GetNegotiatedRtpHeaderExtensions() const override;
-
  private:
   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_;
@@ -348,6 +336,24 @@ class BaseChannel : public ChannelInterface,
   bool was_ever_writable_n_ RTC_GUARDED_BY(network_thread()) = false;
   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
+  // PeerConnection::GetCryptoOptions(), which is const state. It's also only
+  // used to filter header extensions when calling
+  // `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
@@ -382,12 +388,6 @@ class BaseChannel : public ChannelInterface,
   // like in Simulcast.
   // This object is not owned by the channel so it must outlive it.
   rtc::UniqueRandomIdGenerator* const ssrc_generator_;
-
-  // |negotiated_header_extensions_| is read and written to on the signaling
-  // thread from the SdpOfferAnswerHandler class (e.g.
-  // PushdownMediaDescription().
-  RtpHeaderExtensions negotiated_header_extensions_
-      RTC_GUARDED_BY(signaling_thread());
 };
 
 // VoiceChannel is a specialization that adds support for early media, DTMF,

pc/channel_interface.h

@@ -64,9 +64,6 @@ class ChannelInterface {
   //   * A DtlsSrtpTransport for DTLS-SRTP.
   virtual bool SetRtpTransport(webrtc::RtpTransportInternal* rtp_transport) = 0;
 
-  // Returns the last negotiated header extensions.
-  virtual RtpHeaderExtensions GetNegotiatedRtpHeaderExtensions() const = 0;
-
  protected:
   virtual ~ChannelInterface() = default;
 };

pc/rtp_transceiver.cc

@@ -509,10 +509,9 @@ RtpTransceiver::HeaderExtensionsToOffer() const {
 
 std::vector<RtpHeaderExtensionCapability>
 RtpTransceiver::HeaderExtensionsNegotiated() const {
-  if (!channel_)
+  RTC_DCHECK_RUN_ON(thread_);
-    return {};
   std::vector<RtpHeaderExtensionCapability> result;
-  for (const auto& ext : channel_->GetNegotiatedRtpHeaderExtensions()) {
+  for (const auto& ext : negotiated_header_extensions_) {
     result.emplace_back(ext.uri, ext.id, RtpTransceiverDirection::kSendRecv);
   }
   return result;
@@ -562,6 +561,15 @@ RTCError RtpTransceiver::SetOfferedRtpHeaderExtensions(
   return RTCError::OK();
 }
 
+void RtpTransceiver::OnNegotiationUpdate(
+    SdpType sdp_type,
+    const cricket::MediaContentDescription* content) {
+  RTC_DCHECK_RUN_ON(thread_);
+  RTC_DCHECK(content);
+  if (sdp_type == SdpType::kAnswer)
+    negotiated_header_extensions_ = content->rtp_header_extensions();
+}
+
 void RtpTransceiver::SetPeerConnectionClosed() {
   is_pc_closed_ = true;
 }

pc/rtp_transceiver.h

@@ -233,6 +233,16 @@ class RtpTransceiver final
       rtc::ArrayView<const RtpHeaderExtensionCapability>
           header_extensions_to_offer) override;
 
+  // Called on the signaling thread when the local or remote content description
+  // is updated. Used to update the negotiated header extensions.
+  // TODO(tommi): The implementation of this method is currently very simple and
+  // only used for updating the negotiated headers. However, we're planning to
+  // move all the updates done on the channel from the transceiver into this
+  // method. This will happen with the ownership of the channel object being
+  // moved into the transceiver.
+  void OnNegotiationUpdate(SdpType sdp_type,
+                           const cricket::MediaContentDescription* content);
+
  private:
   void OnFirstPacketReceived();
   void StopSendingAndReceiving();
@@ -264,6 +274,13 @@ class RtpTransceiver final
   cricket::ChannelManager* channel_manager_ = nullptr;
   std::vector<RtpCodecCapability> codec_preferences_;
   std::vector<RtpHeaderExtensionCapability> header_extensions_to_offer_;
+
+  // |negotiated_header_extensions_| is read and written to on the signaling
+  // thread from the SdpOfferAnswerHandler class (e.g.
+  // PushdownMediaDescription().
+  cricket::RtpHeaderExtensions negotiated_header_extensions_
+      RTC_GUARDED_BY(thread_);
+
   const std::function<void()> on_negotiation_needed_;
 };
 

pc/rtp_transceiver_unittest.cc

@@ -287,9 +287,6 @@ TEST_F(RtpTransceiverTestForHeaderExtensions,
   EXPECT_CALL(mock_channel, media_type())
       .WillRepeatedly(Return(cricket::MediaType::MEDIA_TYPE_AUDIO));
   EXPECT_CALL(mock_channel, media_channel()).WillRepeatedly(Return(nullptr));
-  cricket::RtpHeaderExtensions extensions;
-  EXPECT_CALL(mock_channel, GetNegotiatedRtpHeaderExtensions)
-      .WillOnce(Return(extensions));
   transceiver_.SetChannel(&mock_channel);
   EXPECT_THAT(transceiver_.HeaderExtensionsNegotiated(), ElementsAre());
 }
@@ -306,10 +303,13 @@ TEST_F(RtpTransceiverTestForHeaderExtensions, ReturnsNegotiatedHdrExts) {
   EXPECT_CALL(mock_channel, media_type())
       .WillRepeatedly(Return(cricket::MediaType::MEDIA_TYPE_AUDIO));
   EXPECT_CALL(mock_channel, media_channel()).WillRepeatedly(Return(nullptr));
+
   cricket::RtpHeaderExtensions extensions = {webrtc::RtpExtension("uri1", 1),
                                              webrtc::RtpExtension("uri2", 2)};
-  EXPECT_CALL(mock_channel, GetNegotiatedRtpHeaderExtensions)
+  cricket::AudioContentDescription description;
-      .WillOnce(Return(extensions));
+  description.set_rtp_header_extensions(extensions);
+  transceiver_.OnNegotiationUpdate(SdpType::kAnswer, &description);
+
   transceiver_.SetChannel(&mock_channel);
   EXPECT_THAT(transceiver_.HeaderExtensionsNegotiated(),
               ElementsAre(RtpHeaderExtensionCapability(
@@ -320,34 +320,27 @@ TEST_F(RtpTransceiverTestForHeaderExtensions, ReturnsNegotiatedHdrExts) {
 
 TEST_F(RtpTransceiverTestForHeaderExtensions,
        ReturnsNegotiatedHdrExtsSecondTime) {
-  EXPECT_CALL(*receiver_.get(), SetMediaChannel(_));
   EXPECT_CALL(*receiver_.get(), StopAndEndTrack());
-  EXPECT_CALL(*sender_.get(), SetMediaChannel(_));
   EXPECT_CALL(*sender_.get(), SetTransceiverAsStopped());
   EXPECT_CALL(*sender_.get(), Stop());
 
-  cricket::MockChannelInterface mock_channel;
-  EXPECT_CALL(mock_channel, SetFirstPacketReceivedCallback(_));
-  EXPECT_CALL(mock_channel, media_type())
-      .WillRepeatedly(Return(cricket::MediaType::MEDIA_TYPE_AUDIO));
-  EXPECT_CALL(mock_channel, media_channel()).WillRepeatedly(Return(nullptr));
-
   cricket::RtpHeaderExtensions extensions = {webrtc::RtpExtension("uri1", 1),
                                              webrtc::RtpExtension("uri2", 2)};
+  cricket::AudioContentDescription description;
+  description.set_rtp_header_extensions(extensions);
+  transceiver_.OnNegotiationUpdate(SdpType::kAnswer, &description);
 
-  EXPECT_CALL(mock_channel, GetNegotiatedRtpHeaderExtensions)
+  EXPECT_THAT(transceiver_.HeaderExtensionsNegotiated(),
-      .WillOnce(Return(extensions));
+              ElementsAre(RtpHeaderExtensionCapability(
-  transceiver_.SetChannel(&mock_channel);
+                              "uri1", 1, RtpTransceiverDirection::kSendRecv),
-  transceiver_.HeaderExtensionsNegotiated();
+                          RtpHeaderExtensionCapability(
-  testing::Mock::VerifyAndClearExpectations(&mock_channel);
+                              "uri2", 2, RtpTransceiverDirection::kSendRecv)));
-  EXPECT_CALL(mock_channel, media_type())
-      .WillRepeatedly(Return(cricket::MediaType::MEDIA_TYPE_AUDIO));
-  EXPECT_CALL(mock_channel, media_channel()).WillRepeatedly(Return(nullptr));
 
   extensions = {webrtc::RtpExtension("uri3", 4),
                 webrtc::RtpExtension("uri5", 6)};
-  EXPECT_CALL(mock_channel, GetNegotiatedRtpHeaderExtensions)
+  description.set_rtp_header_extensions(extensions);
-      .WillOnce(Return(extensions));
+  transceiver_.OnNegotiationUpdate(SdpType::kAnswer, &description);
+
   EXPECT_THAT(transceiver_.HeaderExtensionsNegotiated(),
               ElementsAre(RtpHeaderExtensionCapability(
                               "uri3", 4, RtpTransceiverDirection::kSendRecv),

pc/sdp_offer_answer.cc

@@ -2499,12 +2499,7 @@ RTCError SdpOfferAnswerHandler::UpdateSessionState(
 
   // Update internal objects according to the session description's media
   // descriptions.
-  RTCError error = PushdownMediaDescription(type, source, bundle_groups_by_mid);
+  return PushdownMediaDescription(type, source, bundle_groups_by_mid);
-  if (!error.ok()) {
-    return error;
-  }
-
-  return RTCError::OK();
 }
 
 bool SdpOfferAnswerHandler::ShouldFireNegotiationNeededEvent(
@@ -4191,7 +4186,11 @@ RTCError SdpOfferAnswerHandler::PushdownMediaDescription(
   }
 
   // Push down the new SDP media section for each audio/video transceiver.
-  for (const auto& transceiver : transceivers()->ListInternal()) {
+  auto rtp_transceivers = transceivers()->ListInternal();
+  std::vector<
+      std::pair<cricket::ChannelInterface*, const MediaContentDescription*>>
+      channels;
+  for (const auto& transceiver : rtp_transceivers) {
     const ContentInfo* content_info =
         FindMediaSectionForTransceiver(transceiver, sdesc);
     cricket::ChannelInterface* channel = transceiver->channel();
@@ -4203,12 +4202,28 @@ RTCError SdpOfferAnswerHandler::PushdownMediaDescription(
     if (!content_desc) {
       continue;
     }
-    std::string error;
+
-    bool success = (source == cricket::CS_LOCAL)
+    transceiver->OnNegotiationUpdate(type, content_desc);
-                       ? channel->SetLocalContent(content_desc, type, &error)
+    channels.push_back(std::make_pair(channel, content_desc));
-                       : channel->SetRemoteContent(content_desc, type, &error);
+  }
-    if (!success) {
+
-      LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER, error);
+  if (!channels.empty()) {
+    RTCError error =
+        pc_->worker_thread()->Invoke<RTCError>(RTC_FROM_HERE, [&]() {
+          std::string error;
+          for (const auto& entry : channels) {
+            bool success =
+                (source == cricket::CS_LOCAL)
+                    ? entry.first->SetLocalContent(entry.second, type, &error)
+                    : entry.first->SetRemoteContent(entry.second, type, &error);
+            if (!success) {
+              LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER, error);
+            }
+          }
+          return RTCError::OK();
+        });
+    if (!error.ok()) {
+      return error;
     }
   }
 

pc/test/mock_channel_interface.h

@@ -58,10 +58,6 @@ class MockChannelInterface : public cricket::ChannelInterface {
               SetRtpTransport,
               (webrtc::RtpTransportInternal*),
               (override));
-  MOCK_METHOD(RtpHeaderExtensions,
-              GetNegotiatedRtpHeaderExtensions,
-              (),
-              (const));
 };
 
 }  // namespace cricket