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}
diff --git a/pc/channel.cc b/pc/channel.cc
index f66083f..5cff129 100644
--- a/pc/channel.cc
+++ b/pc/channel.cc
@@ -293,36 +293,17 @@
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");
-
- 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);
- });
+ 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");
-
- 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();
+ return SetRemoteContent_w(content, type, error_desc);
}
bool BaseChannel::SetPayloadTypeDemuxingEnabled(bool enabled) {
@@ -863,11 +844,6 @@
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,
diff --git a/pc/channel.h b/pc/channel.h
index 5c2235e..e9401e4 100644
--- a/pc/channel.h
+++ b/pc/channel.h
@@ -288,13 +288,6 @@
// 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 @@
// 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 @@
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 @@
// 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,
diff --git a/pc/channel_interface.h b/pc/channel_interface.h
index fced8cc..3b71f0f 100644
--- a/pc/channel_interface.h
+++ b/pc/channel_interface.h
@@ -64,9 +64,6 @@
// * 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;
};
diff --git a/pc/rtp_transceiver.cc b/pc/rtp_transceiver.cc
index 15c4c4c..0b7de31 100644
--- a/pc/rtp_transceiver.cc
+++ b/pc/rtp_transceiver.cc
@@ -509,10 +509,9 @@
std::vector<RtpHeaderExtensionCapability>
RtpTransceiver::HeaderExtensionsNegotiated() const {
- if (!channel_)
- return {};
+ RTC_DCHECK_RUN_ON(thread_);
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 @@
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;
}
diff --git a/pc/rtp_transceiver.h b/pc/rtp_transceiver.h
index 32da9af..35dea25 100644
--- a/pc/rtp_transceiver.h
+++ b/pc/rtp_transceiver.h
@@ -233,6 +233,16 @@
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 @@
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_;
};
diff --git a/pc/rtp_transceiver_unittest.cc b/pc/rtp_transceiver_unittest.cc
index 523f307..f9f4f20 100644
--- a/pc/rtp_transceiver_unittest.cc
+++ b/pc/rtp_transceiver_unittest.cc
@@ -287,9 +287,6 @@
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 @@
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)
- .WillOnce(Return(extensions));
+ cricket::AudioContentDescription description;
+ 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,
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)
- .WillOnce(Return(extensions));
- transceiver_.SetChannel(&mock_channel);
- transceiver_.HeaderExtensionsNegotiated();
- testing::Mock::VerifyAndClearExpectations(&mock_channel);
- EXPECT_CALL(mock_channel, media_type())
- .WillRepeatedly(Return(cricket::MediaType::MEDIA_TYPE_AUDIO));
- EXPECT_CALL(mock_channel, media_channel()).WillRepeatedly(Return(nullptr));
+ EXPECT_THAT(transceiver_.HeaderExtensionsNegotiated(),
+ ElementsAre(RtpHeaderExtensionCapability(
+ "uri1", 1, RtpTransceiverDirection::kSendRecv),
+ RtpHeaderExtensionCapability(
+ "uri2", 2, RtpTransceiverDirection::kSendRecv)));
extensions = {webrtc::RtpExtension("uri3", 4),
webrtc::RtpExtension("uri5", 6)};
- EXPECT_CALL(mock_channel, GetNegotiatedRtpHeaderExtensions)
- .WillOnce(Return(extensions));
+ description.set_rtp_header_extensions(extensions);
+ transceiver_.OnNegotiationUpdate(SdpType::kAnswer, &description);
+
EXPECT_THAT(transceiver_.HeaderExtensionsNegotiated(),
ElementsAre(RtpHeaderExtensionCapability(
"uri3", 4, RtpTransceiverDirection::kSendRecv),
diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc
index e3fe002..f3f22fc 100644
--- a/pc/sdp_offer_answer.cc
+++ b/pc/sdp_offer_answer.cc
@@ -2499,12 +2499,7 @@
// Update internal objects according to the session description's media
// descriptions.
- RTCError error = PushdownMediaDescription(type, source, bundle_groups_by_mid);
- if (!error.ok()) {
- return error;
- }
-
- return RTCError::OK();
+ return PushdownMediaDescription(type, source, bundle_groups_by_mid);
}
bool SdpOfferAnswerHandler::ShouldFireNegotiationNeededEvent(
@@ -4191,7 +4186,11 @@
}
// 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 @@
if (!content_desc) {
continue;
}
- std::string error;
- bool success = (source == cricket::CS_LOCAL)
- ? channel->SetLocalContent(content_desc, type, &error)
- : channel->SetRemoteContent(content_desc, type, &error);
- if (!success) {
- LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER, error);
+
+ transceiver->OnNegotiationUpdate(type, content_desc);
+ channels.push_back(std::make_pair(channel, content_desc));
+ }
+
+ 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;
}
}
diff --git a/pc/test/mock_channel_interface.h b/pc/test/mock_channel_interface.h
index d376e42..6faba5c 100644
--- a/pc/test/mock_channel_interface.h
+++ b/pc/test/mock_channel_interface.h
@@ -58,10 +58,6 @@
SetRtpTransport,
(webrtc::RtpTransportInternal*),
(override));
- MOCK_METHOD(RtpHeaderExtensions,
- GetNegotiatedRtpHeaderExtensions,
- (),
- (const));
};
} // namespace cricket
