From 18ee1d55b4cdd7faef75464e39ec06568790adbc Mon Sep 17 00:00:00 2001 From: Steve Anton Date: Mon, 11 Sep 2017 11:32:35 -0700 Subject: [PATCH] Move SDP m= line matching from BaseChannel to WebRtcSession This is part of the work towards implementing Unified Plan. The logic for correlating m= lines to channels is changing in Unified Plan. Moving this logic to WebRtcSession means that we do not need to add a flag to BaseChannel to indicate which logic it should use (i.e., Plan B vs. Unified Plan) and can keep those details in WebRtcSession. Bug: webrtc:8183 Change-Id: I729da73ece01fd20f45e82f8956a02c4cad2469e Reviewed-on: https://chromium-review.googlesource.com/653490 Reviewed-by: Taylor Brandstetter Reviewed-by: Peter Thatcher Commit-Queue: Steve Anton Cr-Commit-Position: refs/heads/master@{#19786} --- webrtc/pc/channel.cc | 50 -------------- webrtc/pc/channel.h | 12 ---- webrtc/pc/channel_unittest.cc | 122 ++++++++++++++++------------------ webrtc/pc/webrtcsession.cc | 55 ++++++++++----- webrtc/pc/webrtcsession.h | 3 + 5 files changed, 101 insertions(+), 141 deletions(-) diff --git a/webrtc/pc/channel.cc b/webrtc/pc/channel.cc index d99a105868..8e829259d9 100644 --- a/webrtc/pc/channel.cc +++ b/webrtc/pc/channel.cc @@ -111,13 +111,6 @@ static bool IsSendContentDirection(MediaContentDirection direction) { return direction == MD_SENDRECV || direction == MD_SENDONLY; } -static const MediaContentDescription* GetContentDescription( - const ContentInfo* cinfo) { - if (cinfo == NULL) - return NULL; - return static_cast(cinfo->description); -} - template void RtpParametersFromMediaDescription( const MediaContentDescriptionImpl* desc, @@ -746,34 +739,6 @@ void BaseChannel::ProcessPacket(bool rtcp, } } -bool BaseChannel::PushdownLocalDescription( - const SessionDescription* local_desc, ContentAction action, - std::string* error_desc) { - const ContentInfo* content_info = GetFirstContent(local_desc); - const MediaContentDescription* content_desc = - GetContentDescription(content_info); - if (content_desc && content_info && !content_info->rejected && - !SetLocalContent(content_desc, action, error_desc)) { - LOG(LS_ERROR) << "Failure in SetLocalContent with action " << action; - return false; - } - return true; -} - -bool BaseChannel::PushdownRemoteDescription( - const SessionDescription* remote_desc, ContentAction action, - std::string* error_desc) { - const ContentInfo* content_info = GetFirstContent(remote_desc); - const MediaContentDescription* content_desc = - GetContentDescription(content_info); - if (content_desc && content_info && !content_info->rejected && - !SetRemoteContent(content_desc, action, error_desc)) { - LOG(LS_ERROR) << "Failure in SetRemoteContent with action " << action; - return false; - } - return true; -} - void BaseChannel::EnableMedia_w() { RTC_DCHECK(worker_thread_ == rtc::Thread::Current()); if (enabled_) @@ -1728,11 +1693,6 @@ void VoiceChannel::UpdateMediaSendRecvState_w() { LOG(LS_INFO) << "Changing voice state, recv=" << recv << " send=" << send; } -const ContentInfo* VoiceChannel::GetFirstContent( - const SessionDescription* sdesc) { - return GetFirstAudioContent(sdesc); -} - bool VoiceChannel::SetLocalContent_w(const MediaContentDescription* content, ContentAction action, std::string* error_desc) { @@ -2015,11 +1975,6 @@ void VideoChannel::StopMediaMonitor() { } } -const ContentInfo* VideoChannel::GetFirstContent( - const SessionDescription* sdesc) { - return GetFirstVideoContent(sdesc); -} - bool VideoChannel::SetLocalContent_w(const MediaContentDescription* content, ContentAction action, std::string* error_desc) { @@ -2201,11 +2156,6 @@ bool RtpDataChannel::SendData(const SendDataParams& params, payload, result)); } -const ContentInfo* RtpDataChannel::GetFirstContent( - const SessionDescription* sdesc) { - return GetFirstDataContent(sdesc); -} - bool RtpDataChannel::CheckDataChannelTypeFromContent( const DataContentDescription* content, std::string* error_desc) { diff --git a/webrtc/pc/channel.h b/webrtc/pc/channel.h index b95bd529b0..4ff21fec08 100644 --- a/webrtc/pc/channel.h +++ b/webrtc/pc/channel.h @@ -121,12 +121,6 @@ class BaseChannel DtlsTransportInternal* rtcp_dtls_transport); void SetTransports(rtc::PacketTransportInternal* rtp_packet_transport, rtc::PacketTransportInternal* rtcp_packet_transport); - bool PushdownLocalDescription(const SessionDescription* local_desc, - ContentAction action, - std::string* error_desc); - bool PushdownRemoteDescription(const SessionDescription* remote_desc, - ContentAction action, - std::string* error_desc); // Channel control bool SetLocalContent(const MediaContentDescription* content, ContentAction action, @@ -302,9 +296,6 @@ class BaseChannel void UpdateMediaSendRecvState(); virtual void UpdateMediaSendRecvState_w() = 0; - // Gets the content info appropriate to the channel (audio or video). - virtual const ContentInfo* GetFirstContent( - const SessionDescription* sdesc) = 0; bool UpdateLocalStreams_w(const std::vector& streams, ContentAction action, std::string* error_desc); @@ -509,7 +500,6 @@ class VoiceChannel : public BaseChannel { rtc::CopyOnWriteBuffer* packet, const rtc::PacketTime& packet_time) override; void UpdateMediaSendRecvState_w() override; - const ContentInfo* GetFirstContent(const SessionDescription* sdesc) override; bool SetLocalContent_w(const MediaContentDescription* content, ContentAction action, std::string* error_desc) override; @@ -589,7 +579,6 @@ class VideoChannel : public BaseChannel { private: // overrides from BaseChannel void UpdateMediaSendRecvState_w() override; - const ContentInfo* GetFirstContent(const SessionDescription* sdesc) override; bool SetLocalContent_w(const MediaContentDescription* content, ContentAction action, std::string* error_desc) override; @@ -699,7 +688,6 @@ class RtpDataChannel : public BaseChannel { typedef rtc::TypedMessageData DataChannelReadyToSendMessageData; // overrides from BaseChannel - const ContentInfo* GetFirstContent(const SessionDescription* sdesc) override; // Checks that data channel type is RTP. bool CheckDataChannelTypeFromContent(const DataContentDescription* content, std::string* error_desc); diff --git a/webrtc/pc/channel_unittest.cc b/webrtc/pc/channel_unittest.cc index 5279683a2b..b16e60d031 100644 --- a/webrtc/pc/channel_unittest.cc +++ b/webrtc/pc/channel_unittest.cc @@ -511,17 +511,13 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> { // overridden in specialized classes } - // Creates a cricket::SessionDescription with one MediaContent and one stream. + // Creates a MediaContent with one stream. // kPcmuCodec is used as audio codec and kH264Codec is used as video codec. - cricket::SessionDescription* CreateSessionDescriptionWithStream( - uint32_t ssrc) { - typename T::Content content; - cricket::SessionDescription* sdesc = new cricket::SessionDescription(); - CreateContent(SECURE, kPcmuCodec, kH264Codec, &content); - AddLegacyStreamInContent(ssrc, 0, &content); - sdesc->AddContent("DUMMY_CONTENT_NAME", - cricket::NS_JINGLE_RTP, content.Copy()); - return sdesc; + typename T::Content* CreateMediaContentWithStream(uint32_t ssrc) { + typename T::Content* content = new typename T::Content(); + CreateContent(SECURE, kPcmuCodec, kH264Codec, content); + AddLegacyStreamInContent(ssrc, 0, content); + return content; } // Will manage the lifetime of a CallThread, making sure it's @@ -1829,41 +1825,39 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> { void TestSetContentFailure() { CreateChannels(0, 0); - auto sdesc = cricket::SessionDescription(); - sdesc.AddContent(cricket::CN_AUDIO, cricket::NS_JINGLE_RTP, - new cricket::AudioContentDescription()); - sdesc.AddContent(cricket::CN_VIDEO, cricket::NS_JINGLE_RTP, - new cricket::VideoContentDescription()); - std::string err; + std::unique_ptr content( + CreateMediaContentWithStream(1)); + media_channel1_->set_fail_set_recv_codecs(true); - EXPECT_FALSE(channel1_->PushdownLocalDescription( - &sdesc, cricket::CA_OFFER, &err)); - EXPECT_FALSE(channel1_->PushdownLocalDescription( - &sdesc, cricket::CA_ANSWER, &err)); + EXPECT_FALSE( + channel1_->SetLocalContent(content.get(), cricket::CA_OFFER, &err)); + EXPECT_FALSE( + channel1_->SetLocalContent(content.get(), cricket::CA_ANSWER, &err)); media_channel1_->set_fail_set_send_codecs(true); - EXPECT_FALSE(channel1_->PushdownRemoteDescription( - &sdesc, cricket::CA_OFFER, &err)); + EXPECT_FALSE( + channel1_->SetRemoteContent(content.get(), cricket::CA_OFFER, &err)); + media_channel1_->set_fail_set_send_codecs(true); - EXPECT_FALSE(channel1_->PushdownRemoteDescription( - &sdesc, cricket::CA_ANSWER, &err)); + EXPECT_FALSE( + channel1_->SetRemoteContent(content.get(), cricket::CA_ANSWER, &err)); } void TestSendTwoOffers() { CreateChannels(0, 0); std::string err; - std::unique_ptr sdesc1( - CreateSessionDescriptionWithStream(1)); - EXPECT_TRUE(channel1_->PushdownLocalDescription( - sdesc1.get(), cricket::CA_OFFER, &err)); + std::unique_ptr content1( + CreateMediaContentWithStream(1)); + EXPECT_TRUE( + channel1_->SetLocalContent(content1.get(), cricket::CA_OFFER, &err)); EXPECT_TRUE(media_channel1_->HasSendStream(1)); - std::unique_ptr sdesc2( - CreateSessionDescriptionWithStream(2)); - EXPECT_TRUE(channel1_->PushdownLocalDescription( - sdesc2.get(), cricket::CA_OFFER, &err)); + std::unique_ptr content2( + CreateMediaContentWithStream(2)); + EXPECT_TRUE( + channel1_->SetLocalContent(content2.get(), cricket::CA_OFFER, &err)); EXPECT_FALSE(media_channel1_->HasSendStream(1)); EXPECT_TRUE(media_channel1_->HasSendStream(2)); } @@ -1872,16 +1866,16 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> { CreateChannels(0, 0); std::string err; - std::unique_ptr sdesc1( - CreateSessionDescriptionWithStream(1)); - EXPECT_TRUE(channel1_->PushdownRemoteDescription( - sdesc1.get(), cricket::CA_OFFER, &err)); + std::unique_ptr content1( + CreateMediaContentWithStream(1)); + EXPECT_TRUE( + channel1_->SetRemoteContent(content1.get(), cricket::CA_OFFER, &err)); EXPECT_TRUE(media_channel1_->HasRecvStream(1)); - std::unique_ptr sdesc2( - CreateSessionDescriptionWithStream(2)); - EXPECT_TRUE(channel1_->PushdownRemoteDescription( - sdesc2.get(), cricket::CA_OFFER, &err)); + std::unique_ptr content2( + CreateMediaContentWithStream(2)); + EXPECT_TRUE( + channel1_->SetRemoteContent(content2.get(), cricket::CA_OFFER, &err)); EXPECT_FALSE(media_channel1_->HasRecvStream(1)); EXPECT_TRUE(media_channel1_->HasRecvStream(2)); } @@ -1891,25 +1885,25 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> { std::string err; // Receive offer - std::unique_ptr sdesc1( - CreateSessionDescriptionWithStream(1)); - EXPECT_TRUE(channel1_->PushdownRemoteDescription( - sdesc1.get(), cricket::CA_OFFER, &err)); + std::unique_ptr content1( + CreateMediaContentWithStream(1)); + EXPECT_TRUE( + channel1_->SetRemoteContent(content1.get(), cricket::CA_OFFER, &err)); EXPECT_TRUE(media_channel1_->HasRecvStream(1)); // Send PR answer - std::unique_ptr sdesc2( - CreateSessionDescriptionWithStream(2)); - EXPECT_TRUE(channel1_->PushdownLocalDescription( - sdesc2.get(), cricket::CA_PRANSWER, &err)); + std::unique_ptr content2( + CreateMediaContentWithStream(2)); + EXPECT_TRUE( + channel1_->SetLocalContent(content2.get(), cricket::CA_PRANSWER, &err)); EXPECT_TRUE(media_channel1_->HasRecvStream(1)); EXPECT_TRUE(media_channel1_->HasSendStream(2)); // Send answer - std::unique_ptr sdesc3( - CreateSessionDescriptionWithStream(3)); - EXPECT_TRUE(channel1_->PushdownLocalDescription( - sdesc3.get(), cricket::CA_ANSWER, &err)); + std::unique_ptr content3( + CreateMediaContentWithStream(3)); + EXPECT_TRUE( + channel1_->SetLocalContent(content3.get(), cricket::CA_ANSWER, &err)); EXPECT_TRUE(media_channel1_->HasRecvStream(1)); EXPECT_FALSE(media_channel1_->HasSendStream(2)); EXPECT_TRUE(media_channel1_->HasSendStream(3)); @@ -1920,25 +1914,25 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> { std::string err; // Send offer - std::unique_ptr sdesc1( - CreateSessionDescriptionWithStream(1)); - EXPECT_TRUE(channel1_->PushdownLocalDescription( - sdesc1.get(), cricket::CA_OFFER, &err)); + std::unique_ptr content1( + CreateMediaContentWithStream(1)); + EXPECT_TRUE( + channel1_->SetLocalContent(content1.get(), cricket::CA_OFFER, &err)); EXPECT_TRUE(media_channel1_->HasSendStream(1)); // Receive PR answer - std::unique_ptr sdesc2( - CreateSessionDescriptionWithStream(2)); - EXPECT_TRUE(channel1_->PushdownRemoteDescription( - sdesc2.get(), cricket::CA_PRANSWER, &err)); + std::unique_ptr content2( + CreateMediaContentWithStream(2)); + EXPECT_TRUE(channel1_->SetRemoteContent(content2.get(), + cricket::CA_PRANSWER, &err)); EXPECT_TRUE(media_channel1_->HasSendStream(1)); EXPECT_TRUE(media_channel1_->HasRecvStream(2)); // Receive answer - std::unique_ptr sdesc3( - CreateSessionDescriptionWithStream(3)); - EXPECT_TRUE(channel1_->PushdownRemoteDescription( - sdesc3.get(), cricket::CA_ANSWER, &err)); + std::unique_ptr content3( + CreateMediaContentWithStream(3)); + EXPECT_TRUE( + channel1_->SetRemoteContent(content3.get(), cricket::CA_ANSWER, &err)); EXPECT_TRUE(media_channel1_->HasSendStream(1)); EXPECT_FALSE(media_channel1_->HasRecvStream(2)); EXPECT_TRUE(media_channel1_->HasRecvStream(3)); diff --git a/webrtc/pc/webrtcsession.cc b/webrtc/pc/webrtcsession.cc index b42366335d..c78dbc4da7 100644 --- a/webrtc/pc/webrtcsession.cc +++ b/webrtc/pc/webrtcsession.cc @@ -847,6 +847,21 @@ bool WebRtcSession::SetRemoteDescription(SessionDescriptionInterface* desc, return true; } +// TODO(steveanton): Eventually it'd be nice to store the channels as a single +// vector of BaseChannel pointers instead of separate voice and video channel +// vectors. At that point, this will become a simple getter. +std::vector WebRtcSession::Channels() const { + std::vector channels; + channels.insert(channels.end(), voice_channels_.begin(), + voice_channels_.end()); + channels.insert(channels.end(), video_channels_.begin(), + video_channels_.end()); + if (rtp_data_channel_) { + channels.push_back(rtp_data_channel_.get()); + } + return channels; +} + void WebRtcSession::LogState(State old_state, State new_state) { LOG(LS_INFO) << "Session:" << id() << " Old state:" << GetStateString(old_state) @@ -953,30 +968,40 @@ bool WebRtcSession::PushdownMediaDescription( cricket::ContentAction action, cricket::ContentSource source, std::string* err) { - auto set_content = [this, action, source, err](cricket::BaseChannel* ch) { - if (!ch) { - return true; - } else if (source == cricket::CS_LOCAL) { - return ch->PushdownLocalDescription(local_description()->description(), - action, err); - } else { - return ch->PushdownRemoteDescription(remote_description()->description(), - action, err); + const SessionDescription* sdesc = + (source == cricket::CS_LOCAL ? local_description() : remote_description()) + ->description(); + RTC_DCHECK(sdesc); + bool all_success = true; + for (auto* channel : Channels()) { + // TODO(steveanton): Add support for multiple channels of the same type. + const ContentInfo* content_info = + cricket::GetFirstMediaContent(sdesc->contents(), channel->media_type()); + if (!content_info) { + continue; } - }; - - bool ret = (set_content(voice_channel()) && set_content(video_channel()) && - set_content(rtp_data_channel())); + const MediaContentDescription* content_desc = + static_cast(content_info->description); + if (content_desc && !content_info->rejected) { + bool success = (source == cricket::CS_LOCAL) + ? channel->SetLocalContent(content_desc, action, err) + : channel->SetRemoteContent(content_desc, action, err); + if (!success) { + all_success = false; + break; + } + } + } // Need complete offer/answer with an SCTP m= section before starting SCTP, // according to https://tools.ietf.org/html/draft-ietf-mmusic-sctp-sdp-19 if (sctp_transport_ && local_description() && remote_description() && cricket::GetFirstDataContent(local_description()->description()) && cricket::GetFirstDataContent(remote_description()->description())) { - ret &= network_thread_->Invoke( + all_success &= network_thread_->Invoke( RTC_FROM_HERE, rtc::Bind(&WebRtcSession::PushdownSctpParameters_n, this, source)); } - return ret; + return all_success; } bool WebRtcSession::PushdownSctpParameters_n(cricket::ContentSource source) { diff --git a/webrtc/pc/webrtcsession.h b/webrtc/pc/webrtcsession.h index 4d52a5c361..686ac1efe2 100644 --- a/webrtc/pc/webrtcsession.h +++ b/webrtc/pc/webrtcsession.h @@ -393,6 +393,9 @@ class WebRtcSession : kAnswer, }; + // Return all managed, non-null channels. + std::vector Channels() const; + // Non-const versions of local_description()/remote_description(), for use // internally. SessionDescriptionInterface* mutable_local_description() {