diff --git a/api/peer_connection_interface.h b/api/peer_connection_interface.h index cc2fa46c99..55cc593759 100644 --- a/api/peer_connection_interface.h +++ b/api/peer_connection_interface.h @@ -991,6 +991,11 @@ class RTC_EXPORT PeerConnectionInterface : public rtc::RefCountInterface { // that this method always takes ownership of it. virtual void SetLocalDescription(SetSessionDescriptionObserver* observer, SessionDescriptionInterface* desc) = 0; + // Implicitly creates an offer or answer (depending on the current signaling + // state) and performs SetLocalDescription() with the newly generated session + // description. + // TODO(hbos): Make pure virtual when implemented by downstream projects. + virtual void SetLocalDescription(SetSessionDescriptionObserver* observer) {} // Sets the remote session description. // The PeerConnection takes the ownership of |desc| even if it fails. // The |observer| callback will be called when done. diff --git a/api/peer_connection_proxy.h b/api/peer_connection_proxy.h index 551af4823e..3b9cf792f4 100644 --- a/api/peer_connection_proxy.h +++ b/api/peer_connection_proxy.h @@ -100,6 +100,7 @@ PROXY_METHOD2(void, SetLocalDescription, SetSessionDescriptionObserver*, SessionDescriptionInterface*) +PROXY_METHOD1(void, SetLocalDescription, SetSessionDescriptionObserver*) PROXY_METHOD2(void, SetRemoteDescription, SetSessionDescriptionObserver*, diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index 7dc2e35aed..46a61ab517 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -694,6 +694,85 @@ class CreateSessionDescriptionObserverOperationWrapper } // namespace +// Used by parameterless SetLocalDescription() to create an offer or answer. +// Upon completion of creating the session description, SetLocalDescription() is +// invoked with the result. +// For consistency with DoSetLocalDescription(), if the PeerConnection is +// destroyed midst operation, we DO NOT inform the +// |set_local_description_observer| that the operation failed. +// TODO(hbos): If/when we process SLD messages in ~PeerConnection, the +// consistent thing would be to inform the observer here. +class PeerConnection::ImplicitCreateSessionDescriptionObserver + : public CreateSessionDescriptionObserver { + public: + ImplicitCreateSessionDescriptionObserver( + rtc::WeakPtr pc, + rtc::scoped_refptr + set_local_description_observer) + : pc_(std::move(pc)), + set_local_description_observer_( + std::move(set_local_description_observer)) {} + ~ImplicitCreateSessionDescriptionObserver() override { + RTC_DCHECK(was_called_); + } + + void SetOperationCompleteCallback( + std::function operation_complete_callback) { + operation_complete_callback_ = std::move(operation_complete_callback); + } + + bool was_called() const { return was_called_; } + + void OnSuccess(SessionDescriptionInterface* desc_ptr) override { + RTC_DCHECK(!was_called_); + std::unique_ptr desc(desc_ptr); + was_called_ = true; + + // Abort early if |pc_| is no longer valid. + if (!pc_) { + operation_complete_callback_(); + return; + } + // DoSetLocalDescription() is currently implemented as a synchronous + // operation but where the |set_local_description_observer_|'s callbacks are + // invoked asynchronously in a post to PeerConnection::OnMessage(). + pc_->DoSetLocalDescription(std::move(desc), + std::move(set_local_description_observer_)); + // For backwards-compatability reasons, we declare the operation as + // completed here (rather than in PeerConnection::OnMessage()). This ensures + // that subsequent offer/answer operations can start immediately (without + // waiting for OnMessage()). + operation_complete_callback_(); + } + + void OnFailure(RTCError error) override { + RTC_DCHECK(!was_called_); + was_called_ = true; + + // Abort early if |pc_| is no longer valid. + if (!pc_) { + operation_complete_callback_(); + return; + } + // DoSetLocalDescription() reports its failures in a post. We do the + // same thing here for consistency. + pc_->PostSetSessionDescriptionFailure( + set_local_description_observer_, + RTCError(error.type(), + std::string("SetLocalDescription failed to create " + "session description - ") + + error.message())); + operation_complete_callback_(); + } + + private: + bool was_called_ = false; + rtc::WeakPtr pc_; + rtc::scoped_refptr + set_local_description_observer_; + std::function operation_complete_callback_; +}; + class PeerConnection::LocalIceCredentialsToReplace { public: // Sets the ICE credentials that need restarting to the ICE credentials of @@ -2382,15 +2461,68 @@ void PeerConnection::SetLocalDescription( // operation but where the |observer|'s callbacks are invoked // asynchronously in a post to OnMessage(). // For backwards-compatability reasons, we declare the operation as - // completed here (rather than in OnMessage()). This ensures that: - // - This operation is not keeping the PeerConnection alive past this - // point. - // - Subsequent offer/answer operations can start immediately (without - // waiting for OnMessage()). + // completed here (rather than in OnMessage()). This ensures that + // subsequent offer/answer operations can start immediately (without + // waiting for OnMessage()). operations_chain_callback(); }); } +void PeerConnection::SetLocalDescription( + SetSessionDescriptionObserver* observer) { + RTC_DCHECK_RUN_ON(signaling_thread()); + // The |create_sdp_observer| handles performing DoSetLocalDescription() with + // the resulting description as well as completing the operation. + rtc::scoped_refptr + create_sdp_observer( + new rtc::RefCountedObject( + weak_ptr_factory_.GetWeakPtr(), + rtc::scoped_refptr(observer))); + // Chain this operation. If asynchronous operations are pending on the chain, + // this operation will be queued to be invoked, otherwise the contents of the + // lambda will execute immediately. + operations_chain_->ChainOperation( + [this_weak_ptr = weak_ptr_factory_.GetWeakPtr(), + create_sdp_observer](std::function operations_chain_callback) { + // The |create_sdp_observer| is responsible for completing the + // operation. + create_sdp_observer->SetOperationCompleteCallback( + std::move(operations_chain_callback)); + // Abort early if |this_weak_ptr| is no longer valid. This triggers the + // same code path as if DoCreateOffer() or DoCreateAnswer() failed. + if (!this_weak_ptr) { + create_sdp_observer->OnFailure(RTCError( + RTCErrorType::INTERNAL_ERROR, + "SetLocalDescription failed because the session was shut down")); + return; + } + switch (this_weak_ptr->signaling_state()) { + case PeerConnectionInterface::kStable: + case PeerConnectionInterface::kHaveLocalOffer: + case PeerConnectionInterface::kHaveRemotePrAnswer: + // TODO(hbos): If [LastCreatedOffer] exists and still represents the + // current state of the system, use that instead of creating another + // offer. + this_weak_ptr->DoCreateOffer(RTCOfferAnswerOptions(), + create_sdp_observer); + break; + case PeerConnectionInterface::kHaveLocalPrAnswer: + case PeerConnectionInterface::kHaveRemoteOffer: + // TODO(hbos): If [LastCreatedAnswer] exists and still represents + // the current state of the system, use that instead of creating + // another answer. + this_weak_ptr->DoCreateAnswer(RTCOfferAnswerOptions(), + create_sdp_observer); + break; + case PeerConnectionInterface::kClosed: + create_sdp_observer->OnFailure(RTCError( + RTCErrorType::INVALID_STATE, + "SetLocalDescription called when PeerConnection is closed.")); + break; + } + }); +} + void PeerConnection::DoSetLocalDescription( std::unique_ptr desc, rtc::scoped_refptr observer) { @@ -2807,11 +2939,9 @@ void PeerConnection::SetRemoteDescription( // the |observer|'s callbacks are invoked asynchronously in a post to // OnMessage(). // For backwards-compatability reasons, we declare the operation as - // completed here (rather than in OnMessage()). This ensures that: - // - This operation is not keeping the PeerConnection alive past this - // point. - // - Subsequent offer/answer operations can start immediately (without - // waiting for OnMessage()). + // completed here (rather than in OnMessage()). This ensures that + // subsequent offer/answer operations can start immediately (without + // waiting for OnMessage()). operations_chain_callback(); }); } diff --git a/pc/peer_connection.h b/pc/peer_connection.h index 7a576f310b..dea05ac318 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -210,6 +210,7 @@ class PeerConnection : public PeerConnectionInternal, const RTCOfferAnswerOptions& options) override; void SetLocalDescription(SetSessionDescriptionObserver* observer, SessionDescriptionInterface* desc) override; + void SetLocalDescription(SetSessionDescriptionObserver* observer) override; void SetRemoteDescription(SetSessionDescriptionObserver* observer, SessionDescriptionInterface* desc) override; void SetRemoteDescription( @@ -314,6 +315,8 @@ class PeerConnection : public PeerConnectionInternal, ~PeerConnection() override; private: + class ImplicitCreateSessionDescriptionObserver; + friend class ImplicitCreateSessionDescriptionObserver; class SetRemoteDescriptionObserverAdapter; friend class SetRemoteDescriptionObserverAdapter; // Represents the [[LocalIceCredentialsToReplace]] internal slot in the spec. diff --git a/pc/peer_connection_signaling_unittest.cc b/pc/peer_connection_signaling_unittest.cc index e79ee3d2e5..30b11ceaa7 100644 --- a/pc/peer_connection_signaling_unittest.cc +++ b/pc/peer_connection_signaling_unittest.cc @@ -565,6 +565,32 @@ TEST_P(PeerConnectionSignalingTest, CloseCreateOfferAndShutdown) { EXPECT_TRUE(observer->called()); } +TEST_P(PeerConnectionSignalingTest, ImplicitCreateOfferAndShutdown) { + auto caller = CreatePeerConnection(); + auto observer = MockSetSessionDescriptionObserver::Create(); + caller->pc()->SetLocalDescription(observer); + caller.reset(nullptr); + EXPECT_FALSE(observer->called()); +} + +TEST_P(PeerConnectionSignalingTest, CloseBeforeImplicitCreateOfferAndShutdown) { + auto caller = CreatePeerConnection(); + auto observer = MockSetSessionDescriptionObserver::Create(); + caller->pc()->Close(); + caller->pc()->SetLocalDescription(observer); + caller.reset(nullptr); + EXPECT_FALSE(observer->called()); +} + +TEST_P(PeerConnectionSignalingTest, CloseAfterImplicitCreateOfferAndShutdown) { + auto caller = CreatePeerConnection(); + auto observer = MockSetSessionDescriptionObserver::Create(); + caller->pc()->SetLocalDescription(observer); + caller->pc()->Close(); + caller.reset(nullptr); + EXPECT_FALSE(observer->called()); +} + TEST_P(PeerConnectionSignalingTest, SetRemoteDescriptionExecutesImmediately) { auto caller = CreatePeerConnectionWithAudioVideo(); auto callee = CreatePeerConnection(); @@ -608,6 +634,143 @@ TEST_P(PeerConnectionSignalingTest, CreateOfferBlocksSetRemoteDescription) { EXPECT_EQ(2u, callee->pc()->GetReceivers().size()); } +TEST_P(PeerConnectionSignalingTest, + ParameterlessSetLocalDescriptionCreatesOffer) { + auto caller = CreatePeerConnectionWithAudioVideo(); + + auto observer = MockSetSessionDescriptionObserver::Create(); + caller->pc()->SetLocalDescription(observer); + + // The offer is created asynchronously; message processing is needed for it to + // complete. + EXPECT_FALSE(observer->called()); + EXPECT_FALSE(caller->pc()->pending_local_description()); + EXPECT_EQ(PeerConnection::kStable, caller->signaling_state()); + + // Wait for messages to be processed. + EXPECT_TRUE_WAIT(observer->called(), kWaitTimeout); + EXPECT_TRUE(observer->result()); + EXPECT_TRUE(caller->pc()->pending_local_description()); + EXPECT_EQ(SdpType::kOffer, + caller->pc()->pending_local_description()->GetType()); + EXPECT_EQ(PeerConnection::kHaveLocalOffer, caller->signaling_state()); +} + +TEST_P(PeerConnectionSignalingTest, + ParameterlessSetLocalDescriptionCreatesAnswer) { + auto caller = CreatePeerConnectionWithAudioVideo(); + auto callee = CreatePeerConnectionWithAudioVideo(); + + callee->SetRemoteDescription(caller->CreateOffer()); + EXPECT_EQ(PeerConnection::kHaveRemoteOffer, callee->signaling_state()); + + auto observer = MockSetSessionDescriptionObserver::Create(); + callee->pc()->SetLocalDescription(observer); + + // The answer is created asynchronously; message processing is needed for it + // to complete. + EXPECT_FALSE(observer->called()); + EXPECT_FALSE(callee->pc()->current_local_description()); + + // Wait for messages to be processed. + EXPECT_TRUE_WAIT(observer->called(), kWaitTimeout); + EXPECT_TRUE(observer->result()); + EXPECT_TRUE(callee->pc()->current_local_description()); + EXPECT_EQ(SdpType::kAnswer, + callee->pc()->current_local_description()->GetType()); + EXPECT_EQ(PeerConnection::kStable, callee->signaling_state()); +} + +TEST_P(PeerConnectionSignalingTest, + ParameterlessSetLocalDescriptionFullExchange) { + auto caller = CreatePeerConnectionWithAudioVideo(); + auto callee = CreatePeerConnectionWithAudioVideo(); + + // SetLocalDescription(), implicitly creating an offer. + rtc::scoped_refptr + caller_set_local_description_observer( + new rtc::RefCountedObject()); + caller->pc()->SetLocalDescription(caller_set_local_description_observer); + EXPECT_TRUE_WAIT(caller_set_local_description_observer->called(), + kWaitTimeout); + ASSERT_TRUE(caller->pc()->pending_local_description()); + + // SetRemoteDescription(offer) + rtc::scoped_refptr + callee_set_remote_description_observer( + new rtc::RefCountedObject()); + callee->pc()->SetRemoteDescription( + callee_set_remote_description_observer.get(), + CloneSessionDescription(caller->pc()->pending_local_description()) + .release()); + + // SetLocalDescription(), implicitly creating an answer. + rtc::scoped_refptr + callee_set_local_description_observer( + new rtc::RefCountedObject()); + callee->pc()->SetLocalDescription(callee_set_local_description_observer); + EXPECT_TRUE_WAIT(callee_set_local_description_observer->called(), + kWaitTimeout); + // Chaining guarantees SetRemoteDescription() happened before + // SetLocalDescription(). + EXPECT_TRUE(callee_set_remote_description_observer->called()); + EXPECT_TRUE(callee->pc()->current_local_description()); + + // SetRemoteDescription(answer) + rtc::scoped_refptr + caller_set_remote_description_observer( + new rtc::RefCountedObject()); + caller->pc()->SetRemoteDescription( + caller_set_remote_description_observer, + CloneSessionDescription(callee->pc()->current_local_description()) + .release()); + EXPECT_TRUE_WAIT(caller_set_remote_description_observer->called(), + kWaitTimeout); + + EXPECT_EQ(PeerConnection::kStable, caller->signaling_state()); + EXPECT_EQ(PeerConnection::kStable, callee->signaling_state()); +} + +TEST_P(PeerConnectionSignalingTest, + ParameterlessSetLocalDescriptionCloseBeforeCreatingOffer) { + auto caller = CreatePeerConnectionWithAudioVideo(); + + auto observer = MockSetSessionDescriptionObserver::Create(); + caller->pc()->Close(); + caller->pc()->SetLocalDescription(observer); + + // The operation should fail asynchronously. + EXPECT_FALSE(observer->called()); + EXPECT_TRUE_WAIT(observer->called(), kWaitTimeout); + EXPECT_FALSE(observer->result()); + // This did not affect the signaling state. + EXPECT_EQ(PeerConnection::kClosed, caller->pc()->signaling_state()); + EXPECT_EQ( + "SetLocalDescription failed to create session description - " + "SetLocalDescription called when PeerConnection is closed.", + observer->error()); +} + +TEST_P(PeerConnectionSignalingTest, + ParameterlessSetLocalDescriptionCloseWhileCreatingOffer) { + auto caller = CreatePeerConnectionWithAudioVideo(); + + auto observer = MockSetSessionDescriptionObserver::Create(); + caller->pc()->SetLocalDescription(observer); + caller->pc()->Close(); + + // The operation should fail asynchronously. + EXPECT_FALSE(observer->called()); + EXPECT_TRUE_WAIT(observer->called(), kWaitTimeout); + EXPECT_FALSE(observer->result()); + // This did not affect the signaling state. + EXPECT_EQ(PeerConnection::kClosed, caller->pc()->signaling_state()); + EXPECT_EQ( + "SetLocalDescription failed to create session description - " + "CreateOffer failed because the session was shut down", + observer->error()); +} + INSTANTIATE_TEST_SUITE_P(PeerConnectionSignalingTest, PeerConnectionSignalingTest, Values(SdpSemantics::kPlanB, diff --git a/pc/test/mock_peer_connection_observers.h b/pc/test/mock_peer_connection_observers.h index 5a388bd91a..2017735dc7 100644 --- a/pc/test/mock_peer_connection_observers.h +++ b/pc/test/mock_peer_connection_observers.h @@ -271,6 +271,10 @@ class MockCreateSessionDescriptionObserver class MockSetSessionDescriptionObserver : public webrtc::SetSessionDescriptionObserver { public: + static rtc::scoped_refptr Create() { + return new rtc::RefCountedObject(); + } + MockSetSessionDescriptionObserver() : called_(false), error_("MockSetSessionDescriptionObserver not called") {}