diff --git a/webrtc/api/peerconnection.cc b/webrtc/api/peerconnection.cc index 6d7d07d027..cdf5a97b68 100644 --- a/webrtc/api/peerconnection.cc +++ b/webrtc/api/peerconnection.cc @@ -1381,6 +1381,26 @@ const SessionDescriptionInterface* PeerConnection::remote_description() const { return session_->remote_description(); } +const SessionDescriptionInterface* PeerConnection::current_local_description() + const { + return session_->current_local_description(); +} + +const SessionDescriptionInterface* PeerConnection::current_remote_description() + const { + return session_->current_remote_description(); +} + +const SessionDescriptionInterface* PeerConnection::pending_local_description() + const { + return session_->pending_local_description(); +} + +const SessionDescriptionInterface* PeerConnection::pending_remote_description() + const { + return session_->pending_remote_description(); +} + void PeerConnection::Close() { TRACE_EVENT0("webrtc", "PeerConnection::Close"); // Update stats here so that we have the most recent stats for tracks and diff --git a/webrtc/api/peerconnection.h b/webrtc/api/peerconnection.h index 70fd86779f..5269e3ae49 100644 --- a/webrtc/api/peerconnection.h +++ b/webrtc/api/peerconnection.h @@ -113,6 +113,12 @@ class PeerConnection : public PeerConnectionInterface, const SessionDescriptionInterface* local_description() const override; const SessionDescriptionInterface* remote_description() const override; + const SessionDescriptionInterface* current_local_description() const override; + const SessionDescriptionInterface* current_remote_description() + const override; + const SessionDescriptionInterface* pending_local_description() const override; + const SessionDescriptionInterface* pending_remote_description() + const override; // JSEP01 // Deprecated, use version without constraints. diff --git a/webrtc/api/peerconnectioninterface.h b/webrtc/api/peerconnectioninterface.h index 9c2301ca19..e81eee2fa0 100644 --- a/webrtc/api/peerconnectioninterface.h +++ b/webrtc/api/peerconnectioninterface.h @@ -493,6 +493,25 @@ class PeerConnectionInterface : public rtc::RefCountInterface { virtual const SessionDescriptionInterface* local_description() const = 0; virtual const SessionDescriptionInterface* remote_description() const = 0; + // A "current" description the one currently negotiated from a complete + // offer/answer exchange. + virtual const SessionDescriptionInterface* current_local_description() const { + return nullptr; + } + virtual const SessionDescriptionInterface* current_remote_description() + const { + return nullptr; + } + // A "pending" description is one that's part of an incomplete offer/answer + // exchange (thus, either an offer or a pranswer). Once the offer/answer + // exchange is finished, the "pending" description will become "current". + virtual const SessionDescriptionInterface* pending_local_description() const { + return nullptr; + } + virtual const SessionDescriptionInterface* pending_remote_description() + const { + return nullptr; + } // Create a new offer. // The CreateSessionDescriptionObserver callback will be called when done. diff --git a/webrtc/api/peerconnectioninterface_unittest.cc b/webrtc/api/peerconnectioninterface_unittest.cc index 7b33c0ce81..c52df41f2b 100644 --- a/webrtc/api/peerconnectioninterface_unittest.cc +++ b/webrtc/api/peerconnectioninterface_unittest.cc @@ -2893,6 +2893,76 @@ TEST_F(PeerConnectionInterfaceTest, SetConfigurationCausingPartialIceRestart) { EXPECT_NE(initial_ufrags[1], subsequent_ufrags[1]); } +// Tests that the methods to return current/pending descriptions work as +// expected at different points in the offer/answer exchange. This test does +// one offer/answer exchange as the offerer, then another as the answerer. +TEST_F(PeerConnectionInterfaceTest, CurrentAndPendingDescriptions) { + // This disables DTLS so we can apply an answer to ourselves. + CreatePeerConnection(); + + // Create initial local offer and get SDP (which will also be used as + // answer/pranswer); + std::unique_ptr offer; + ASSERT_TRUE(DoCreateOffer(&offer, nullptr)); + std::string sdp; + EXPECT_TRUE(offer->ToString(&sdp)); + + // Set local offer. + SessionDescriptionInterface* local_offer = offer.release(); + EXPECT_TRUE(DoSetLocalDescription(local_offer)); + EXPECT_EQ(local_offer, pc_->pending_local_description()); + EXPECT_EQ(nullptr, pc_->pending_remote_description()); + EXPECT_EQ(nullptr, pc_->current_local_description()); + EXPECT_EQ(nullptr, pc_->current_remote_description()); + + // Set remote pranswer. + SessionDescriptionInterface* remote_pranswer = + webrtc::CreateSessionDescription(SessionDescriptionInterface::kPrAnswer, + sdp, nullptr); + EXPECT_TRUE(DoSetRemoteDescription(remote_pranswer)); + EXPECT_EQ(local_offer, pc_->pending_local_description()); + EXPECT_EQ(remote_pranswer, pc_->pending_remote_description()); + EXPECT_EQ(nullptr, pc_->current_local_description()); + EXPECT_EQ(nullptr, pc_->current_remote_description()); + + // Set remote answer. + SessionDescriptionInterface* remote_answer = webrtc::CreateSessionDescription( + SessionDescriptionInterface::kAnswer, sdp, nullptr); + EXPECT_TRUE(DoSetRemoteDescription(remote_answer)); + EXPECT_EQ(nullptr, pc_->pending_local_description()); + EXPECT_EQ(nullptr, pc_->pending_remote_description()); + EXPECT_EQ(local_offer, pc_->current_local_description()); + EXPECT_EQ(remote_answer, pc_->current_remote_description()); + + // Set remote offer. + SessionDescriptionInterface* remote_offer = webrtc::CreateSessionDescription( + SessionDescriptionInterface::kOffer, sdp, nullptr); + EXPECT_TRUE(DoSetRemoteDescription(remote_offer)); + EXPECT_EQ(remote_offer, pc_->pending_remote_description()); + EXPECT_EQ(nullptr, pc_->pending_local_description()); + EXPECT_EQ(local_offer, pc_->current_local_description()); + EXPECT_EQ(remote_answer, pc_->current_remote_description()); + + // Set local pranswer. + SessionDescriptionInterface* local_pranswer = + webrtc::CreateSessionDescription(SessionDescriptionInterface::kPrAnswer, + sdp, nullptr); + EXPECT_TRUE(DoSetLocalDescription(local_pranswer)); + EXPECT_EQ(remote_offer, pc_->pending_remote_description()); + EXPECT_EQ(local_pranswer, pc_->pending_local_description()); + EXPECT_EQ(local_offer, pc_->current_local_description()); + EXPECT_EQ(remote_answer, pc_->current_remote_description()); + + // Set local answer. + SessionDescriptionInterface* local_answer = webrtc::CreateSessionDescription( + SessionDescriptionInterface::kAnswer, sdp, nullptr); + EXPECT_TRUE(DoSetLocalDescription(local_answer)); + EXPECT_EQ(nullptr, pc_->pending_remote_description()); + EXPECT_EQ(nullptr, pc_->pending_local_description()); + EXPECT_EQ(remote_offer, pc_->current_remote_description()); + EXPECT_EQ(local_answer, pc_->current_local_description()); +} + class PeerConnectionMediaConfigTest : public testing::Test { protected: void SetUp() override { diff --git a/webrtc/api/peerconnectionproxy.h b/webrtc/api/peerconnectionproxy.h index f002014846..2110ee2a50 100644 --- a/webrtc/api/peerconnectionproxy.h +++ b/webrtc/api/peerconnectionproxy.h @@ -47,6 +47,14 @@ BEGIN_SIGNALING_PROXY_MAP(PeerConnection) CreateDataChannel, const std::string&, const DataChannelInit*) PROXY_CONSTMETHOD0(const SessionDescriptionInterface*, local_description) PROXY_CONSTMETHOD0(const SessionDescriptionInterface*, remote_description) + PROXY_CONSTMETHOD0(const SessionDescriptionInterface*, + pending_local_description) + PROXY_CONSTMETHOD0(const SessionDescriptionInterface*, + pending_remote_description) + PROXY_CONSTMETHOD0(const SessionDescriptionInterface*, + current_local_description) + PROXY_CONSTMETHOD0(const SessionDescriptionInterface*, + current_remote_description) PROXY_METHOD2(void, CreateOffer, CreateSessionDescriptionObserver*, const MediaConstraintsInterface*) PROXY_METHOD2(void, CreateAnswer, CreateSessionDescriptionObserver*, diff --git a/webrtc/api/webrtcsession.cc b/webrtc/api/webrtcsession.cc index ea724a344c..5d414baac2 100644 --- a/webrtc/api/webrtcsession.cc +++ b/webrtc/api/webrtcsession.cc @@ -623,7 +623,7 @@ cricket::SecurePolicy WebRtcSession::SdesPolicy() const { bool WebRtcSession::GetSslRole(const std::string& transport_name, rtc::SSLRole* role) { - if (!local_desc_ || !remote_desc_) { + if (!local_description() || !remote_description()) { LOG(LS_INFO) << "Local and Remote descriptions must be applied to get " << "SSL Role of the session."; return false; @@ -669,25 +669,31 @@ bool WebRtcSession::SetLocalDescription(SessionDescriptionInterface* desc, transport_controller_->SetIceRole(cricket::ICEROLE_CONTROLLING); } - local_desc_.reset(desc_temp.release()); + if (action == kAnswer) { + current_local_description_.reset(desc_temp.release()); + pending_local_description_.reset(nullptr); + current_remote_description_.reset(pending_remote_description_.release()); + } else { + pending_local_description_.reset(desc_temp.release()); + } // Transport and Media channels will be created only when offer is set. - if (action == kOffer && !CreateChannels(local_desc_->description())) { + if (action == kOffer && !CreateChannels(local_description()->description())) { // TODO(mallinath) - Handle CreateChannel failure, as new local description // is applied. Restore back to old description. return BadLocalSdp(desc->type(), kCreateChannelFailed, err_desc); } // Remove unused channels if MediaContentDescription is rejected. - RemoveUnusedChannels(local_desc_->description()); + RemoveUnusedChannels(local_description()->description()); if (!UpdateSessionState(action, cricket::CS_LOCAL, err_desc)) { return false; } - if (remote_desc_) { + if (remote_description()) { // Now that we have a local description, we can push down remote candidates. - UseCandidatesInSessionDescription(remote_desc_.get()); + UseCandidatesInSessionDescription(remote_description()); } pending_ice_restarts_.clear(); @@ -710,12 +716,25 @@ bool WebRtcSession::SetRemoteDescription(SessionDescriptionInterface* desc, return false; } - std::unique_ptr old_remote_desc( - remote_desc_.release()); - remote_desc_.reset(desc_temp.release()); + const SessionDescriptionInterface* old_remote_description = + remote_description(); + // Grab ownership of the description being replaced for the remainder of this + // method, since it's used below. + std::unique_ptr replaced_remote_description; + Action action = GetAction(desc->type()); + if (action == kAnswer) { + replaced_remote_description.reset( + pending_remote_description_ ? pending_remote_description_.release() + : current_remote_description_.release()); + current_remote_description_.reset(desc_temp.release()); + pending_remote_description_.reset(nullptr); + current_local_description_.reset(pending_local_description_.release()); + } else { + replaced_remote_description.reset(pending_remote_description_.release()); + pending_remote_description_.reset(desc_temp.release()); + } // Transport and Media channels will be created only when offer is set. - Action action = GetAction(desc->type()); if (action == kOffer && !CreateChannels(desc->description())) { // TODO(mallinath) - Handle CreateChannel failure, as new local description // is applied. Restore back to old description. @@ -731,19 +750,20 @@ bool WebRtcSession::SetRemoteDescription(SessionDescriptionInterface* desc, return false; } - if (local_desc_ && !UseCandidatesInSessionDescription(desc)) { + if (local_description() && !UseCandidatesInSessionDescription(desc)) { return BadRemoteSdp(desc->type(), kInvalidCandidates, err_desc); } - if (old_remote_desc) { + if (old_remote_description) { for (const cricket::ContentInfo& content : - old_remote_desc->description()->contents()) { + old_remote_description->description()->contents()) { // Check if this new SessionDescription contains new ICE ufrag and // password that indicates the remote peer requests an ICE restart. // TODO(deadbeef): When we start storing both the current and pending // remote description, this should reset pending_ice_restarts and compare // against the current description. - if (CheckForRemoteIceRestart(old_remote_desc.get(), desc, content.name)) { + if (CheckForRemoteIceRestart(old_remote_description, desc, + content.name)) { if (action == kOffer) { pending_ice_restarts_.insert(content.name); } @@ -756,7 +776,7 @@ bool WebRtcSession::SetRemoteDescription(SessionDescriptionInterface* desc, // description plus any candidates added since then. We should remove // this once we're sure it won't break anything. WebRtcSessionDescriptionFactory::CopyCandidatesFromSessionDescription( - old_remote_desc.get(), content.name, desc); + old_remote_description, content.name, desc); } } } @@ -839,9 +859,11 @@ bool WebRtcSession::UpdateSessionState( } } else if (action == kAnswer) { const cricket::ContentGroup* local_bundle = - local_desc_->description()->GetGroupByName(cricket::GROUP_TYPE_BUNDLE); + local_description()->description()->GetGroupByName( + cricket::GROUP_TYPE_BUNDLE); const cricket::ContentGroup* remote_bundle = - remote_desc_->description()->GetGroupByName(cricket::GROUP_TYPE_BUNDLE); + remote_description()->description()->GetGroupByName( + cricket::GROUP_TYPE_BUNDLE); if (local_bundle && remote_bundle) { // The answerer decides the transport to bundle on. const cricket::ContentGroup* answer_bundle = @@ -888,11 +910,11 @@ bool WebRtcSession::PushdownMediaDescription( if (!ch) { return true; } else if (source == cricket::CS_LOCAL) { - return ch->PushdownLocalDescription(local_desc_->description(), action, - err); + return ch->PushdownLocalDescription(local_description()->description(), + action, err); } else { - return ch->PushdownRemoteDescription(remote_desc_->description(), action, - err); + return ch->PushdownRemoteDescription(remote_description()->description(), + action, err); } }; @@ -907,11 +929,11 @@ bool WebRtcSession::PushdownTransportDescription(cricket::ContentSource source, RTC_DCHECK(signaling_thread()->IsCurrent()); if (source == cricket::CS_LOCAL) { - return PushdownLocalTransportDescription(local_desc_->description(), action, - error_desc); + return PushdownLocalTransportDescription(local_description()->description(), + action, error_desc); } - return PushdownRemoteTransportDescription(remote_desc_->description(), action, - error_desc); + return PushdownRemoteTransportDescription(remote_description()->description(), + action, error_desc); } bool WebRtcSession::PushdownLocalTransportDescription( @@ -1059,7 +1081,7 @@ bool WebRtcSession::EnableBundle(const cricket::ContentGroup& bundle) { } bool WebRtcSession::ProcessIceMessage(const IceCandidateInterface* candidate) { - if (!remote_desc_) { + if (!remote_description()) { LOG(LS_ERROR) << "ProcessIceMessage: ICE candidates can't be added " << "without any remote session description."; return false; @@ -1077,7 +1099,7 @@ bool WebRtcSession::ProcessIceMessage(const IceCandidateInterface* candidate) { } // Add this candidate to the remote session description. - if (!remote_desc_->AddCandidate(candidate)) { + if (!mutable_remote_description()->AddCandidate(candidate)) { LOG(LS_ERROR) << "ProcessIceMessage: Candidate cannot be used."; return false; } @@ -1092,7 +1114,7 @@ bool WebRtcSession::ProcessIceMessage(const IceCandidateInterface* candidate) { bool WebRtcSession::RemoveRemoteIceCandidates( const std::vector& candidates) { - if (!remote_desc_) { + if (!remote_description()) { LOG(LS_ERROR) << "RemoveRemoteIceCandidates: ICE candidates can't be " << "removed without any remote session description."; return false; @@ -1103,7 +1125,8 @@ bool WebRtcSession::RemoveRemoteIceCandidates( return false; } - size_t number_removed = remote_desc_->RemoveCandidates(candidates); + size_t number_removed = + mutable_remote_description()->RemoveCandidates(candidates); if (number_removed != candidates.size()) { LOG(LS_ERROR) << "RemoveRemoteIceCandidates: Failed to remove candidates. " << "Requested " << candidates.size() << " but only " @@ -1157,18 +1180,20 @@ void WebRtcSession::MaybeStartGathering() { bool WebRtcSession::GetLocalTrackIdBySsrc(uint32_t ssrc, std::string* track_id) { - if (!local_desc_) { + if (!local_description()) { return false; } - return webrtc::GetTrackIdBySsrc(local_desc_->description(), ssrc, track_id); + return webrtc::GetTrackIdBySsrc(local_description()->description(), ssrc, + track_id); } bool WebRtcSession::GetRemoteTrackIdBySsrc(uint32_t ssrc, std::string* track_id) { - if (!remote_desc_) { + if (!remote_description()) { return false; } - return webrtc::GetTrackIdBySsrc(remote_desc_->description(), ssrc, track_id); + return webrtc::GetTrackIdBySsrc(remote_description()->description(), ssrc, + track_id); } std::string WebRtcSession::BadStateErrMsg(State state) { @@ -1186,8 +1211,8 @@ bool WebRtcSession::CanInsertDtmf(const std::string& track_id) { uint32_t send_ssrc = 0; // The Dtmf is negotiated per channel not ssrc, so we only check if the ssrc // exists. - if (!local_desc_ || - !GetAudioSsrcByTrackId(local_desc_->description(), track_id, + if (!local_description() || + !GetAudioSsrcByTrackId(local_description()->description(), track_id, &send_ssrc)) { LOG(LS_ERROR) << "CanInsertDtmf: Track does not exist: " << track_id; return false; @@ -1203,8 +1228,9 @@ bool WebRtcSession::InsertDtmf(const std::string& track_id, return false; } uint32_t send_ssrc = 0; - if (!VERIFY(local_desc_ && GetAudioSsrcByTrackId(local_desc_->description(), - track_id, &send_ssrc))) { + if (!VERIFY(local_description() && + GetAudioSsrcByTrackId(local_description()->description(), + track_id, &send_ssrc))) { LOG(LS_ERROR) << "InsertDtmf: Track does not exist: " << track_id; return false; } @@ -1402,8 +1428,8 @@ void WebRtcSession::OnTransportControllerCandidatesGathered( if (ice_observer_) { ice_observer_->OnIceCandidate(&candidate); } - if (local_desc_) { - local_desc_->AddCandidate(&candidate); + if (local_description()) { + mutable_local_description()->AddCandidate(&candidate); } } } @@ -1421,8 +1447,8 @@ void WebRtcSession::OnTransportControllerCandidatesRemoved( } } - if (local_desc_) { - local_desc_->RemoveCandidates(candidates); + if (local_description()) { + mutable_local_description()->RemoveCandidates(candidates); } if (ice_observer_) { ice_observer_->OnIceCandidatesRemoved(candidates); @@ -1444,12 +1470,12 @@ void WebRtcSession::EnableChannels() { // Returns the media index for a local ice candidate given the content name. bool WebRtcSession::GetLocalCandidateMediaIndex(const std::string& content_name, int* sdp_mline_index) { - if (!local_desc_ || !sdp_mline_index) { + if (!local_description() || !sdp_mline_index) { return false; } bool content_found = false; - const ContentInfos& contents = local_desc_->description()->contents(); + const ContentInfos& contents = local_description()->description()->contents(); for (size_t index = 0; index < contents.size(); ++index) { if (contents[index].name == content_name) { *sdp_mline_index = static_cast(index); @@ -1490,14 +1516,15 @@ bool WebRtcSession::UseCandidatesInSessionDescription( bool WebRtcSession::UseCandidate(const IceCandidateInterface* candidate) { size_t mediacontent_index = static_cast(candidate->sdp_mline_index()); - size_t remote_content_size = remote_desc_->description()->contents().size(); + size_t remote_content_size = + remote_description()->description()->contents().size(); if (mediacontent_index >= remote_content_size) { LOG(LS_ERROR) << "UseCandidate: Invalid candidate media index."; return false; } cricket::ContentInfo content = - remote_desc_->description()->contents()[mediacontent_index]; + remote_description()->description()->contents()[mediacontent_index]; std::vector candidates; candidates.push_back(candidate->candidate()); // Invoking BaseSession method to handle remote candidates. @@ -1835,8 +1862,8 @@ bool WebRtcSession::ValidateSessionDescription( // Verify m-lines in Answer when compared against Offer. if (action == kAnswer) { const cricket::SessionDescription* offer_desc = - (source == cricket::CS_LOCAL) ? remote_desc_->description() - : local_desc_->description(); + (source == cricket::CS_LOCAL) ? remote_description()->description() + : local_description()->description(); if (!VerifyMediaDescriptions(sdesc->description(), offer_desc)) { return BadAnswerSdp(source, kMlineMismatch, err_desc); } @@ -1890,7 +1917,7 @@ bool WebRtcSession::ReadyToUseRemoteCandidate( *valid = true; const SessionDescriptionInterface* current_remote_desc = - remote_desc ? remote_desc : remote_desc_.get(); + remote_desc ? remote_desc : remote_description(); if (!current_remote_desc) { return false; diff --git a/webrtc/api/webrtcsession.h b/webrtc/api/webrtcsession.h index fa46ada883..ef31560509 100644 --- a/webrtc/api/webrtcsession.h +++ b/webrtc/api/webrtcsession.h @@ -247,10 +247,24 @@ class WebRtcSession : void MaybeStartGathering(); const SessionDescriptionInterface* local_description() const { - return local_desc_.get(); + return pending_local_description_ ? pending_local_description_.get() + : current_local_description_.get(); } const SessionDescriptionInterface* remote_description() const { - return remote_desc_.get(); + return pending_remote_description_ ? pending_remote_description_.get() + : current_remote_description_.get(); + } + const SessionDescriptionInterface* current_local_description() const { + return current_local_description_.get(); + } + const SessionDescriptionInterface* current_remote_description() const { + return current_remote_description_.get(); + } + const SessionDescriptionInterface* pending_local_description() const { + return pending_local_description_.get(); + } + const SessionDescriptionInterface* pending_remote_description() const { + return pending_remote_description_.get(); } // Get the id used as a media stream track's "id" field from ssrc. @@ -354,6 +368,17 @@ class WebRtcSession : kAnswer, }; + // Non-const versions of local_description()/remote_description(), for use + // internally. + SessionDescriptionInterface* mutable_local_description() { + return pending_local_description_ ? pending_local_description_.get() + : current_local_description_.get(); + } + SessionDescriptionInterface* mutable_remote_description() { + return pending_remote_description_ ? pending_remote_description_.get() + : current_remote_description_.get(); + } + // Log session state. void LogState(State old_state, State new_state); @@ -519,8 +544,10 @@ class WebRtcSession : IceObserver* ice_observer_; PeerConnectionInterface::IceConnectionState ice_connection_state_; bool ice_connection_receiving_; - std::unique_ptr local_desc_; - std::unique_ptr remote_desc_; + std::unique_ptr current_local_description_; + std::unique_ptr pending_local_description_; + std::unique_ptr current_remote_description_; + std::unique_ptr pending_remote_description_; // If the remote peer is using a older version of implementation. bool older_version_remote_peer_; bool dtls_enabled_;