diff --git a/pc/channel.cc b/pc/channel.cc index f66083f0e4..5cff129c6e 100644 --- a/pc/channel.cc +++ b/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"); - - SetContent_s(content, type); - - return InvokeOnWorker(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(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 @@ 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, diff --git a/pc/channel.h b/pc/channel.h index 5c2235e9ee..e9401e4e4e 100644 --- a/pc/channel.h +++ b/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 - T InvokeOnWorker(const rtc::Location& posted_from, - rtc::FunctionView functor) { - return worker_thread_->Invoke(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, diff --git a/pc/channel_interface.h b/pc/channel_interface.h index fced8cc267..3b71f0f8b5 100644 --- a/pc/channel_interface.h +++ b/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; }; diff --git a/pc/rtp_transceiver.cc b/pc/rtp_transceiver.cc index 15c4c4c4d1..0b7de31372 100644 --- a/pc/rtp_transceiver.cc +++ b/pc/rtp_transceiver.cc @@ -509,10 +509,9 @@ RtpTransceiver::HeaderExtensionsToOffer() const { std::vector RtpTransceiver::HeaderExtensionsNegotiated() const { - if (!channel_) - return {}; + RTC_DCHECK_RUN_ON(thread_); std::vector 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; } diff --git a/pc/rtp_transceiver.h b/pc/rtp_transceiver.h index 32da9af601..35dea25a7b 100644 --- a/pc/rtp_transceiver.h +++ b/pc/rtp_transceiver.h @@ -233,6 +233,16 @@ class RtpTransceiver final rtc::ArrayView 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 codec_preferences_; std::vector 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 on_negotiation_needed_; }; diff --git a/pc/rtp_transceiver_unittest.cc b/pc/rtp_transceiver_unittest.cc index 523f30737a..f9f4f205ea 100644 --- a/pc/rtp_transceiver_unittest.cc +++ b/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) - .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, 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) - .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 e3fe002207..f3f22fc208 100644 --- a/pc/sdp_offer_answer.cc +++ b/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); - if (!error.ok()) { - return error; - } - - return RTCError::OK(); + return PushdownMediaDescription(type, source, bundle_groups_by_mid); } 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> + 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) - ? 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(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 d376e423d5..6faba5c8fc 100644 --- a/pc/test/mock_channel_interface.h +++ b/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