From a6d2444c84004d10a5d8b8517bbd178600f8412f Mon Sep 17 00:00:00 2001 From: Peter Thatcher Date: Thu, 9 Jul 2015 21:26:36 -0700 Subject: [PATCH] Remove BaseSession::SignalNewDescription. It was only used by GTP and now just clutters the code. R=pbos@webrtc.org Review URL: https://codereview.webrtc.org/1228203002 . Cr-Commit-Position: refs/heads/master@{#9564} --- talk/app/webrtc/webrtcsession.h | 13 --- talk/session/media/channel.cc | 23 ---- talk/session/media/channel_unittest.cc | 156 ++++++++++--------------- webrtc/p2p/base/session.cc | 49 -------- webrtc/p2p/base/session.h | 5 - 5 files changed, 64 insertions(+), 182 deletions(-) diff --git a/talk/app/webrtc/webrtcsession.h b/talk/app/webrtc/webrtcsession.h index a2cba20970..30ebc1e49e 100644 --- a/talk/app/webrtc/webrtcsession.h +++ b/talk/app/webrtc/webrtcsession.h @@ -258,19 +258,6 @@ class WebRtcSession : public cricket::BaseSession, metrics_observer_ = metrics_observer; } - protected: - // Don't fire a new description. The only thing it's used for is to - // push new media descriptions to the BaseChannels. But in - // WebRtcSession, we just push to the BaseChannels directly, so we - // don't need this (and it would cause the descriptions to be pushed - // down twice). - // TODO(pthatcher): Remove this method and signal completely from - // BaseSession once all the subclasses of BaseSession push to - // BaseChannels directly rather than relying on the signal, or once - // BaseChannel no longer listens to the event and requires - // descriptions to be pushed down. - virtual void SignalNewDescription() override {} - private: // Indicates the type of SessionDescription in a call to SetLocalDescription // and SetRemoteDescription. diff --git a/talk/session/media/channel.cc b/talk/session/media/channel.cc index 0134d343d2..d30972db06 100644 --- a/talk/session/media/channel.cc +++ b/talk/session/media/channel.cc @@ -204,11 +204,6 @@ bool BaseChannel::Init() { return false; } - session_->SignalNewLocalDescription.connect( - this, &BaseChannel::OnNewLocalDescription); - session_->SignalNewRemoteDescription.connect( - this, &BaseChannel::OnNewRemoteDescription); - // Both RTP and RTCP channels are set, we can call SetInterface on // media channel and it can set network options. media_channel_->SetInterface(this); @@ -663,24 +658,6 @@ void BaseChannel::HandlePacket(bool rtcp, rtc::Buffer* packet, } } -void BaseChannel::OnNewLocalDescription( - BaseSession* session, ContentAction action) { - std::string error_desc; - if (!PushdownLocalDescription( - session->local_description(), action, &error_desc)) { - SetSessionError(session_, BaseSession::ERROR_CONTENT, error_desc); - } -} - -void BaseChannel::OnNewRemoteDescription( - BaseSession* session, ContentAction action) { - std::string error_desc; - if (!PushdownRemoteDescription( - session->remote_description(), action, &error_desc)) { - SetSessionError(session_, BaseSession::ERROR_CONTENT, error_desc); - } -} - bool BaseChannel::PushdownLocalDescription( const SessionDescription* local_desc, ContentAction action, std::string* error_desc) { diff --git a/talk/session/media/channel_unittest.cc b/talk/session/media/channel_unittest.cc index 4a82f551c3..2573454b22 100644 --- a/talk/session/media/channel_unittest.cc +++ b/talk/session/media/channel_unittest.cc @@ -1559,62 +1559,42 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> { void TestSetContentFailure() { CreateChannels(0, 0); - typename T::Content content; - cricket::SessionDescription* sdesc_loc = new cricket::SessionDescription(); - cricket::SessionDescription* sdesc_rem = new cricket::SessionDescription(); - // Set up the session description. - CreateContent(0, kPcmuCodec, kH264Codec, &content); - sdesc_loc->AddContent(cricket::CN_AUDIO, cricket::NS_JINGLE_RTP, - new cricket::AudioContentDescription()); - sdesc_loc->AddContent(cricket::CN_VIDEO, cricket::NS_JINGLE_RTP, - new cricket::VideoContentDescription()); - session1_.set_local_description(sdesc_loc); - sdesc_rem->AddContent(cricket::CN_AUDIO, cricket::NS_JINGLE_RTP, - new cricket::AudioContentDescription()); - sdesc_rem->AddContent(cricket::CN_VIDEO, cricket::NS_JINGLE_RTP, - new cricket::VideoContentDescription()); - session1_.set_remote_description(sdesc_rem); + 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()); - // Test failures in SetLocalContent. + std::string err; media_channel1_->set_fail_set_recv_codecs(true); - session1_.SetError(cricket::BaseSession::ERROR_NONE, ""); - session1_.SetState(cricket::BaseSession::STATE_SENTINITIATE); - EXPECT_EQ(cricket::BaseSession::ERROR_CONTENT, session1_.error()); - media_channel1_->set_fail_set_recv_codecs(true); - session1_.SetError(cricket::BaseSession::ERROR_NONE, ""); - session1_.SetState(cricket::BaseSession::STATE_SENTACCEPT); - EXPECT_EQ(cricket::BaseSession::ERROR_CONTENT, session1_.error()); + EXPECT_FALSE(channel1_->PushdownLocalDescription( + &sdesc, cricket::CA_OFFER, &err)); + EXPECT_FALSE(channel1_->PushdownLocalDescription( + &sdesc, cricket::CA_ANSWER, &err)); - // Test failures in SetRemoteContent. media_channel1_->set_fail_set_send_codecs(true); - session1_.SetError(cricket::BaseSession::ERROR_NONE, ""); - session1_.SetState(cricket::BaseSession::STATE_RECEIVEDINITIATE); - EXPECT_EQ(cricket::BaseSession::ERROR_CONTENT, session1_.error()); + EXPECT_FALSE(channel1_->PushdownRemoteDescription( + &sdesc, cricket::CA_OFFER, &err)); media_channel1_->set_fail_set_send_codecs(true); - session1_.SetError(cricket::BaseSession::ERROR_NONE, ""); - session1_.SetState(cricket::BaseSession::STATE_RECEIVEDACCEPT); - EXPECT_EQ(cricket::BaseSession::ERROR_CONTENT, session1_.error()); + EXPECT_FALSE(channel1_->PushdownRemoteDescription( + &sdesc, cricket::CA_ANSWER, &err)); } void TestSendTwoOffers() { CreateChannels(0, 0); - // Set up the initial session description. - cricket::SessionDescription* sdesc = CreateSessionDescriptionWithStream(1); - session1_.set_local_description(sdesc); - - session1_.SetError(cricket::BaseSession::ERROR_NONE, ""); - session1_.SetState(cricket::BaseSession::STATE_SENTINITIATE); - EXPECT_EQ(cricket::BaseSession::ERROR_NONE, session1_.error()); + std::string err; + rtc::scoped_ptr sdesc1( + CreateSessionDescriptionWithStream(1)); + EXPECT_TRUE(channel1_->PushdownLocalDescription( + sdesc1.get(), cricket::CA_OFFER, &err)); EXPECT_TRUE(media_channel1_->HasSendStream(1)); - // Update the local description and set the state again. - sdesc = CreateSessionDescriptionWithStream(2); - session1_.set_local_description(sdesc); - - session1_.SetState(cricket::BaseSession::STATE_SENTINITIATE); - EXPECT_EQ(cricket::BaseSession::ERROR_NONE, session1_.error()); + rtc::scoped_ptr sdesc2( + CreateSessionDescriptionWithStream(2)); + EXPECT_TRUE(channel1_->PushdownLocalDescription( + sdesc2.get(), cricket::CA_OFFER, &err)); EXPECT_FALSE(media_channel1_->HasSendStream(1)); EXPECT_TRUE(media_channel1_->HasSendStream(2)); } @@ -1622,19 +1602,17 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> { void TestReceiveTwoOffers() { CreateChannels(0, 0); - // Set up the initial session description. - cricket::SessionDescription* sdesc = CreateSessionDescriptionWithStream(1); - session1_.set_remote_description(sdesc); - - session1_.SetError(cricket::BaseSession::ERROR_NONE, ""); - session1_.SetState(cricket::BaseSession::STATE_RECEIVEDINITIATE); - EXPECT_EQ(cricket::BaseSession::ERROR_NONE, session1_.error()); + std::string err; + rtc::scoped_ptr sdesc1( + CreateSessionDescriptionWithStream(1)); + EXPECT_TRUE(channel1_->PushdownRemoteDescription( + sdesc1.get(), cricket::CA_OFFER, &err)); EXPECT_TRUE(media_channel1_->HasRecvStream(1)); - sdesc = CreateSessionDescriptionWithStream(2); - session1_.set_remote_description(sdesc); - session1_.SetState(cricket::BaseSession::STATE_RECEIVEDINITIATE); - EXPECT_EQ(cricket::BaseSession::ERROR_NONE, session1_.error()); + rtc::scoped_ptr sdesc2( + CreateSessionDescriptionWithStream(2)); + EXPECT_TRUE(channel1_->PushdownRemoteDescription( + sdesc2.get(), cricket::CA_OFFER, &err)); EXPECT_FALSE(media_channel1_->HasRecvStream(1)); EXPECT_TRUE(media_channel1_->HasRecvStream(2)); } @@ -1642,30 +1620,27 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> { void TestSendPrAnswer() { CreateChannels(0, 0); - // Set up the initial session description. - cricket::SessionDescription* sdesc = CreateSessionDescriptionWithStream(1); - session1_.set_remote_description(sdesc); - - session1_.SetError(cricket::BaseSession::ERROR_NONE, ""); - session1_.SetState(cricket::BaseSession::STATE_RECEIVEDINITIATE); - EXPECT_EQ(cricket::BaseSession::ERROR_NONE, session1_.error()); + std::string err; + // Receive offer + rtc::scoped_ptr sdesc1( + CreateSessionDescriptionWithStream(1)); + EXPECT_TRUE(channel1_->PushdownRemoteDescription( + sdesc1.get(), cricket::CA_OFFER, &err)); EXPECT_TRUE(media_channel1_->HasRecvStream(1)); - // Send PRANSWER - sdesc = CreateSessionDescriptionWithStream(2); - session1_.set_local_description(sdesc); - - session1_.SetState(cricket::BaseSession::STATE_SENTPRACCEPT); - EXPECT_EQ(cricket::BaseSession::ERROR_NONE, session1_.error()); + // Send PR answer + rtc::scoped_ptr sdesc2( + CreateSessionDescriptionWithStream(2)); + EXPECT_TRUE(channel1_->PushdownLocalDescription( + sdesc2.get(), cricket::CA_PRANSWER, &err)); EXPECT_TRUE(media_channel1_->HasRecvStream(1)); EXPECT_TRUE(media_channel1_->HasSendStream(2)); - // Send ACCEPT - sdesc = CreateSessionDescriptionWithStream(3); - session1_.set_local_description(sdesc); - - session1_.SetState(cricket::BaseSession::STATE_SENTACCEPT); - EXPECT_EQ(cricket::BaseSession::ERROR_NONE, session1_.error()); + // Send answer + rtc::scoped_ptr sdesc3( + CreateSessionDescriptionWithStream(3)); + EXPECT_TRUE(channel1_->PushdownLocalDescription( + sdesc3.get(), cricket::CA_ANSWER, &err)); EXPECT_TRUE(media_channel1_->HasRecvStream(1)); EXPECT_FALSE(media_channel1_->HasSendStream(2)); EXPECT_TRUE(media_channel1_->HasSendStream(3)); @@ -1674,30 +1649,27 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> { void TestReceivePrAnswer() { CreateChannels(0, 0); - // Set up the initial session description. - cricket::SessionDescription* sdesc = CreateSessionDescriptionWithStream(1); - session1_.set_local_description(sdesc); - - session1_.SetError(cricket::BaseSession::ERROR_NONE, ""); - session1_.SetState(cricket::BaseSession::STATE_SENTINITIATE); - EXPECT_EQ(cricket::BaseSession::ERROR_NONE, session1_.error()); + std::string err; + // Send offer + rtc::scoped_ptr sdesc1( + CreateSessionDescriptionWithStream(1)); + EXPECT_TRUE(channel1_->PushdownLocalDescription( + sdesc1.get(), cricket::CA_OFFER, &err)); EXPECT_TRUE(media_channel1_->HasSendStream(1)); - // Receive PRANSWER - sdesc = CreateSessionDescriptionWithStream(2); - session1_.set_remote_description(sdesc); - - session1_.SetState(cricket::BaseSession::STATE_RECEIVEDPRACCEPT); - EXPECT_EQ(cricket::BaseSession::ERROR_NONE, session1_.error()); + // Receive PR answer + rtc::scoped_ptr sdesc2( + CreateSessionDescriptionWithStream(2)); + EXPECT_TRUE(channel1_->PushdownRemoteDescription( + sdesc2.get(), cricket::CA_PRANSWER, &err)); EXPECT_TRUE(media_channel1_->HasSendStream(1)); EXPECT_TRUE(media_channel1_->HasRecvStream(2)); - // Receive ACCEPT - sdesc = CreateSessionDescriptionWithStream(3); - session1_.set_remote_description(sdesc); - - session1_.SetState(cricket::BaseSession::STATE_RECEIVEDACCEPT); - EXPECT_EQ(cricket::BaseSession::ERROR_NONE, session1_.error()); + // Receive answer + rtc::scoped_ptr sdesc3( + CreateSessionDescriptionWithStream(3)); + EXPECT_TRUE(channel1_->PushdownRemoteDescription( + sdesc3.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/p2p/base/session.cc b/webrtc/p2p/base/session.cc index 55b1d90c1a..ca0525dd85 100644 --- a/webrtc/p2p/base/session.cc +++ b/webrtc/p2p/base/session.cc @@ -582,7 +582,6 @@ void BaseSession::SetState(State state) { SignalState(this, state_); signaling_thread_->Post(this, MSG_STATE); } - SignalNewDescription(); } void BaseSession::SetError(Error error, const std::string& error_desc) { @@ -787,54 +786,6 @@ bool BaseSession::GetTransportDescription(const SessionDescription* description, return true; } -void BaseSession::SignalNewDescription() { - ContentAction action; - ContentSource source; - if (!GetContentAction(&action, &source)) { - return; - } - if (source == CS_LOCAL) { - SignalNewLocalDescription(this, action); - } else { - SignalNewRemoteDescription(this, action); - } -} - -bool BaseSession::GetContentAction(ContentAction* action, - ContentSource* source) { - switch (state_) { - // new local description - case STATE_SENTINITIATE: - *action = CA_OFFER; - *source = CS_LOCAL; - break; - case STATE_SENTPRACCEPT: - *action = CA_PRANSWER; - *source = CS_LOCAL; - break; - case STATE_SENTACCEPT: - *action = CA_ANSWER; - *source = CS_LOCAL; - break; - // new remote description - case STATE_RECEIVEDINITIATE: - *action = CA_OFFER; - *source = CS_REMOTE; - break; - case STATE_RECEIVEDPRACCEPT: - *action = CA_PRANSWER; - *source = CS_REMOTE; - break; - case STATE_RECEIVEDACCEPT: - *action = CA_ANSWER; - *source = CS_REMOTE; - break; - default: - return false; - } - return true; -} - void BaseSession::OnMessage(rtc::Message *pmsg) { switch (pmsg->message_id) { case MSG_TIMEOUT: diff --git a/webrtc/p2p/base/session.h b/webrtc/p2p/base/session.h index 6214dad0b1..8f9e308a86 100644 --- a/webrtc/p2p/base/session.h +++ b/webrtc/p2p/base/session.h @@ -412,8 +412,6 @@ class BaseSession : public sigslot::has_slots<>, Error error_; std::string error_desc_; - // Fires the new description signal according to the current state. - virtual void SignalNewDescription(); // This method will delete the Transport and TransportChannelImpls // and replace those with the Transport object of the first // MediaContent in bundle_group. @@ -439,9 +437,6 @@ class BaseSession : public sigslot::has_slots<>, const std::string& content_name, TransportDescription* info); - // Gets the ContentAction and ContentSource according to the session state. - bool GetContentAction(ContentAction* action, ContentSource* source); - rtc::Thread* const signaling_thread_; rtc::Thread* const worker_thread_; PortAllocator* const port_allocator_;