diff --git a/api/BUILD.gn b/api/BUILD.gn index 03460f6e70..278ab1536e 100644 --- a/api/BUILD.gn +++ b/api/BUILD.gn @@ -63,6 +63,8 @@ rtc_static_library("libjingle_peerconnection_api") { "rtpreceiverinterface.h", "rtpsenderinterface.h", "rtptransceiverinterface.h", + "setremotedescriptionobserverinterface.cc", + "setremotedescriptionobserverinterface.h", "statstypes.cc", "statstypes.h", "turncustomizer.h", @@ -379,6 +381,7 @@ if (rtc_include_tests) { deps = [ ":array_view", ":libjingle_peerconnection_api", + ":libjingle_peerconnection_test_api", ":optional", ":ortc_api", "../rtc_base:rtc_base_approved", diff --git a/api/peerconnectioninterface.h b/api/peerconnectioninterface.h index a682459a13..0cff5df2d8 100644 --- a/api/peerconnectioninterface.h +++ b/api/peerconnectioninterface.h @@ -82,6 +82,7 @@ #include "api/rtceventlogoutput.h" #include "api/rtpreceiverinterface.h" #include "api/rtpsenderinterface.h" +#include "api/setremotedescriptionobserverinterface.h" #include "api/stats/rtcstatscollectorcallback.h" #include "api/statstypes.h" #include "api/turncustomizer.h" @@ -726,8 +727,18 @@ class PeerConnectionInterface : public rtc::RefCountInterface { // Sets the remote session description. // The PeerConnection takes the ownership of |desc| even if it fails. // The |observer| callback will be called when done. + // TODO(hbos): Remove when Chrome implements the new signature. virtual void SetRemoteDescription(SetSessionDescriptionObserver* observer, - SessionDescriptionInterface* desc) = 0; + SessionDescriptionInterface* desc) { + SetRemoteDescription( + std::unique_ptr(desc), + rtc::scoped_refptr( + new SetRemoteDescriptionObserverAdapter(observer))); + } + // TODO(hbos): Make pure virtual when Chrome has updated its signature. + virtual void SetRemoteDescription( + std::unique_ptr desc, + rtc::scoped_refptr observer) {} // Deprecated; Replaced by SetConfiguration. // TODO(deadbeef): Remove once Chrome is moved over to SetConfiguration. virtual bool UpdateIce(const IceServers& configuration, diff --git a/api/peerconnectionproxy.h b/api/peerconnectionproxy.h index 36ee50f9d0..bdd9fcb16d 100644 --- a/api/peerconnectionproxy.h +++ b/api/peerconnectionproxy.h @@ -88,6 +88,10 @@ BEGIN_SIGNALING_PROXY_MAP(PeerConnection) SetRemoteDescription, SetSessionDescriptionObserver*, SessionDescriptionInterface*) + PROXY_METHOD2(void, + SetRemoteDescription, + std::unique_ptr, + rtc::scoped_refptr); PROXY_METHOD0(PeerConnectionInterface::RTCConfiguration, GetConfiguration); PROXY_METHOD2(bool, SetConfiguration, diff --git a/api/setremotedescriptionobserverinterface.cc b/api/setremotedescriptionobserverinterface.cc new file mode 100644 index 0000000000..67006a3900 --- /dev/null +++ b/api/setremotedescriptionobserverinterface.cc @@ -0,0 +1,61 @@ +/* + * Copyright 2017 The WebRTC project authors. All Rights Reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +#include "api/setremotedescriptionobserverinterface.h" + +#include + +namespace webrtc { + +// The message keeps the observer alive through reference counting. +class SetRemoteDescriptionObserverAdapter::MessageData + : public rtc::MessageData { + public: + static MessageData* Create( + rtc::scoped_refptr observer, + RTCError error) { + return new MessageData(std::move(observer), std::move(error)); + } + + const RTCError& error() const { return error_; } + + private: + MessageData(rtc::scoped_refptr observer, + RTCError error) + : observer_(std::move(observer)), error_(std::move(error)) {} + + rtc::scoped_refptr observer_; + RTCError error_; +}; + +SetRemoteDescriptionObserverAdapter::SetRemoteDescriptionObserverAdapter( + rtc::scoped_refptr wrapper) + : wrapper_(std::move(wrapper)) {} + +void SetRemoteDescriptionObserverAdapter::OnSetRemoteDescriptionComplete( + RTCError error) { + // MessageData keeps a reference to |this|, ensuring it is not deleted until + // OnMessage(). + rtc::Thread::Current()->Post(RTC_FROM_HERE, this, 0, + MessageData::Create(this, std::move(error))); +} + +void SetRemoteDescriptionObserverAdapter::OnMessage(rtc::Message* msg) { + MessageData* message = static_cast(msg->pdata); + if (message->error().ok()) + wrapper_->OnSuccess(); + else + wrapper_->OnFailure(message->error().message()); + // Delete the message data, this may delete |this|. Don't use member variables + // after this line. + delete message; +} + +} // namespace webrtc diff --git a/api/setremotedescriptionobserverinterface.h b/api/setremotedescriptionobserverinterface.h new file mode 100644 index 0000000000..f1b65a1f1f --- /dev/null +++ b/api/setremotedescriptionobserverinterface.h @@ -0,0 +1,69 @@ +/* + * Copyright 2017 The WebRTC project authors. All Rights Reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +#ifndef API_SETREMOTEDESCRIPTIONOBSERVERINTERFACE_H_ +#define API_SETREMOTEDESCRIPTIONOBSERVERINTERFACE_H_ + +#include +#include + +#include "api/jsep.h" +#include "api/mediastreaminterface.h" +#include "api/rtcerror.h" +#include "api/rtpreceiverinterface.h" +#include "rtc_base/bind.h" +#include "rtc_base/messagehandler.h" +#include "rtc_base/refcount.h" +#include "rtc_base/refcountedobject.h" +#include "rtc_base/scoped_ref_ptr.h" +#include "rtc_base/thread.h" + +namespace webrtc { + +// An observer for PeerConnectionInterface::SetRemoteDescription(). The +// callback is invoked such that the state of the peer connection can be +// examined to accurately reflect the effects of the SetRemoteDescription +// operation. +class SetRemoteDescriptionObserverInterface : public rtc::RefCountInterface { + public: + // On success, |error.ok()| is true. + virtual void OnSetRemoteDescriptionComplete(RTCError error) = 0; +}; + +// Upon completion, posts a task to execute the callback of the +// SetSessionDescriptionObserver asynchronously on the same thread. At this +// point, the state of the peer connection might no longer reflect the effects +// of the SetRemoteDescription operation, as the peer connection could have been +// modified during the post. +// TODO(hbos): Remove this class once we remove the version of +// PeerConnectionInterface::SetRemoteDescription() that takes a +// SetSessionDescriptionObserver as an argument. +class SetRemoteDescriptionObserverAdapter + : public rtc::RefCountedObject, + public rtc::MessageHandler { + public: + SetRemoteDescriptionObserverAdapter( + rtc::scoped_refptr wrapper); + + // SetRemoteDescriptionObserverInterface implementation. + void OnSetRemoteDescriptionComplete(RTCError error) override; + + // rtc::MessageHandler implementation. + void OnMessage(rtc::Message* msg) override; + + private: + class MessageData; + + rtc::scoped_refptr wrapper_; +}; + +} // namespace webrtc + +#endif // API_SETREMOTEDESCRIPTIONOBSERVERINTERFACE_H_ diff --git a/pc/BUILD.gn b/pc/BUILD.gn index d4ee2a533a..1c524d299d 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn @@ -402,6 +402,7 @@ if (rtc_include_tests) { "rtcstatscollector_unittest.cc", "rtpsenderreceiver_unittest.cc", "sctputils_unittest.cc", + "setremotedescriptionobserverinterface_unittest.cc", "statscollector_unittest.cc", "test/fakeaudiocapturemodule_unittest.cc", "test/testsdpstrings.h", @@ -443,6 +444,7 @@ if (rtc_include_tests) { "..:webrtc_common", "../api:fakemetricsobserver", "../api:libjingle_peerconnection_test_api", + "../api:optional", "../api:rtc_stats_api", "../api/audio_codecs:audio_codecs_api", "../api/audio_codecs:builtin_audio_decoder_factory", diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index 62e0ef868f..08370d9f61 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -1463,7 +1463,7 @@ void PeerConnection::SetLocalDescription( std::string error; // Takes the ownership of |desc_temp|. On success, local_description() is // updated to reflect the description that was passed in. - if (!SetLocalDescription(std::move(desc_temp), &error)) { + if (!SetCurrentOrPendingLocalDescription(std::move(desc_temp), &error)) { PostSetSessionDescriptionFailure(observer, error); return; } @@ -1541,26 +1541,25 @@ void PeerConnection::SetLocalDescription( } void PeerConnection::SetRemoteDescription( - SetSessionDescriptionObserver* observer, - SessionDescriptionInterface* desc) { + std::unique_ptr desc, + rtc::scoped_refptr observer) { TRACE_EVENT0("webrtc", "PeerConnection::SetRemoteDescription"); if (!observer) { RTC_LOG(LS_ERROR) << "SetRemoteDescription - observer is NULL."; return; } if (!desc) { - PostSetSessionDescriptionFailure(observer, "SessionDescription is NULL."); + observer->OnSetRemoteDescriptionComplete(RTCError( + RTCErrorType::UNSUPPORTED_PARAMETER, "SessionDescription is NULL.")); return; } - // Takes the ownership of |desc| regardless of the result. - std::unique_ptr desc_temp(desc); - if (IsClosed()) { - std::string error = "Failed to set remote " + desc_temp->type() + + std::string error = "Failed to set remote " + desc->type() + " sdp: Called in wrong state: STATE_CLOSED"; RTC_LOG(LS_ERROR) << error; - PostSetSessionDescriptionFailure(observer, error); + observer->OnSetRemoteDescriptionComplete( + RTCError(RTCErrorType::INVALID_STATE, std::move(error))); return; } @@ -1568,10 +1567,11 @@ void PeerConnection::SetRemoteDescription( // streams that might be removed by updating the session description. stats_->UpdateStats(kStatsOutputLevelStandard); std::string error; - // Takes the ownership of |desc_temp|. On success, remote_description() is - // updated to reflect the description that was passed in. - if (!SetRemoteDescription(std::move(desc_temp), &error)) { - PostSetSessionDescriptionFailure(observer, error); + // Takes the ownership of |desc|. On success, remote_description() is updated + // to reflect the description that was passed in. + if (!SetCurrentOrPendingRemoteDescription(std::move(desc), &error)) { + observer->OnSetRemoteDescriptionComplete( + RTCError(RTCErrorType::UNSUPPORTED_PARAMETER, std::move(error))); return; } RTC_DCHECK(remote_description()); @@ -1661,9 +1661,7 @@ void PeerConnection::SetRemoteDescription( UpdateEndedRemoteMediaStreams(); - SetSessionDescriptionMsg* msg = new SetSessionDescriptionMsg(observer); - signaling_thread()->Post(RTC_FROM_HERE, this, - MSG_SET_SESSIONDESCRIPTION_SUCCESS, msg); + observer->OnSetRemoteDescriptionComplete(RTCError::OK()); if (remote_description()->type() == SessionDescriptionInterface::kAnswer) { // TODO(deadbeef): We already had to hop to the network thread for @@ -3260,7 +3258,7 @@ bool PeerConnection::GetSslRole(const std::string& content_name, role); } -bool PeerConnection::SetLocalDescription( +bool PeerConnection::SetCurrentOrPendingLocalDescription( std::unique_ptr desc, std::string* err_desc) { RTC_DCHECK(signaling_thread()->IsCurrent()); @@ -3316,7 +3314,7 @@ bool PeerConnection::SetLocalDescription( return true; } -bool PeerConnection::SetRemoteDescription( +bool PeerConnection::SetCurrentOrPendingRemoteDescription( std::unique_ptr desc, std::string* err_desc) { RTC_DCHECK(signaling_thread()->IsCurrent()); diff --git a/pc/peerconnection.h b/pc/peerconnection.h index 2b36a86e49..a67780b796 100644 --- a/pc/peerconnection.h +++ b/pc/peerconnection.h @@ -150,8 +150,10 @@ class PeerConnection : public PeerConnectionInterface, const RTCOfferAnswerOptions& options) override; void SetLocalDescription(SetSessionDescriptionObserver* observer, SessionDescriptionInterface* desc) override; - void SetRemoteDescription(SetSessionDescriptionObserver* observer, - SessionDescriptionInterface* desc) override; + void SetRemoteDescription( + std::unique_ptr desc, + rtc::scoped_refptr observer) + override; PeerConnectionInterface::RTCConfiguration GetConfiguration() override; bool SetConfiguration( const PeerConnectionInterface::RTCConfiguration& configuration, @@ -539,10 +541,15 @@ class PeerConnection : public PeerConnectionInterface, // Get current SSL role used by SCTP's underlying transport. bool GetSctpSslRole(rtc::SSLRole* role); - bool SetLocalDescription(std::unique_ptr desc, - std::string* err_desc); - bool SetRemoteDescription(std::unique_ptr desc, - std::string* err_desc); + // Validates and takes ownership of the description, setting it as the current + // or pending description (depending on the description's action) if it is + // valid. Also updates ice role, candidates, creates and destroys channels. + bool SetCurrentOrPendingLocalDescription( + std::unique_ptr desc, + std::string* err_desc); + bool SetCurrentOrPendingRemoteDescription( + std::unique_ptr desc, + std::string* err_desc); cricket::IceConfig ParseIceConfig( const PeerConnectionInterface::RTCConfiguration& config) const; diff --git a/pc/peerconnection_rtp_unittest.cc b/pc/peerconnection_rtp_unittest.cc index 860c6dc50e..ae7b67d690 100644 --- a/pc/peerconnection_rtp_unittest.cc +++ b/pc/peerconnection_rtp_unittest.cc @@ -33,6 +33,25 @@ namespace { +const uint32_t kDefaultTimeout = 10000u; + +template +class OnSuccessObserver : public rtc::RefCountedObject< + webrtc::SetRemoteDescriptionObserverInterface> { + public: + explicit OnSuccessObserver(MethodFunctor on_success) + : on_success_(std::move(on_success)) {} + + // webrtc::SetRemoteDescriptionObserverInterface implementation. + void OnSetRemoteDescriptionComplete(webrtc::RTCError error) override { + RTC_CHECK(error.ok()); + on_success_(); + } + + private: + MethodFunctor on_success_; +}; + class PeerConnectionRtpTest : public testing::Test { public: PeerConnectionRtpTest() @@ -60,25 +79,31 @@ class PeerConnectionRtpTest : public testing::Test { rtc::scoped_refptr pc_factory_; }; -TEST_F(PeerConnectionRtpTest, AddTrackWithoutStreamFiresOnAddTrack) { +// These tests cover |webrtc::PeerConnectionObserver| callbacks firing upon +// setting the remote description. +class PeerConnectionRtpCallbacksTest : public PeerConnectionRtpTest {}; + +TEST_F(PeerConnectionRtpCallbacksTest, AddTrackWithoutStreamFiresOnAddTrack) { auto caller = CreatePeerConnection(); auto callee = CreatePeerConnection(); rtc::scoped_refptr audio_track( pc_factory_->CreateAudioTrack("audio_track", nullptr)); EXPECT_TRUE(caller->pc()->AddTrack(audio_track.get(), {})); - ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + ASSERT_TRUE( + callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), + static_cast(nullptr))); - ASSERT_EQ(1u, callee->observer()->add_track_events_.size()); - // TODO(deadbeef): When no stream is handled correctly we would expect + ASSERT_EQ(callee->observer()->add_track_events_.size(), 1u); + // TODO(hbos): When "no stream" is handled correctly we would expect // |add_track_events_[0].streams| to be empty. https://crbug.com/webrtc/7933 auto& add_track_event = callee->observer()->add_track_events_[0]; - ASSERT_EQ(1u, add_track_event.streams.size()); + ASSERT_EQ(add_track_event.streams.size(), 1u); EXPECT_TRUE(add_track_event.streams[0]->FindAudioTrack("audio_track")); EXPECT_EQ(add_track_event.streams, add_track_event.receiver->streams()); } -TEST_F(PeerConnectionRtpTest, AddTrackWithStreamFiresOnAddTrack) { +TEST_F(PeerConnectionRtpCallbacksTest, AddTrackWithStreamFiresOnAddTrack) { auto caller = CreatePeerConnection(); auto callee = CreatePeerConnection(); @@ -86,34 +111,42 @@ TEST_F(PeerConnectionRtpTest, AddTrackWithStreamFiresOnAddTrack) { pc_factory_->CreateAudioTrack("audio_track", nullptr)); auto stream = webrtc::MediaStream::Create("audio_stream"); EXPECT_TRUE(caller->pc()->AddTrack(audio_track.get(), {stream.get()})); - ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + ASSERT_TRUE( + callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), + static_cast(nullptr))); - ASSERT_EQ(1u, callee->observer()->add_track_events_.size()); + ASSERT_EQ(callee->observer()->add_track_events_.size(), 1u); auto& add_track_event = callee->observer()->add_track_events_[0]; - ASSERT_EQ(1u, add_track_event.streams.size()); + ASSERT_EQ(add_track_event.streams.size(), 1u); EXPECT_EQ("audio_stream", add_track_event.streams[0]->label()); EXPECT_TRUE(add_track_event.streams[0]->FindAudioTrack("audio_track")); EXPECT_EQ(add_track_event.streams, add_track_event.receiver->streams()); } -TEST_F(PeerConnectionRtpTest, RemoveTrackWithoutStreamFiresOnRemoveTrack) { +TEST_F(PeerConnectionRtpCallbacksTest, + RemoveTrackWithoutStreamFiresOnRemoveTrack) { auto caller = CreatePeerConnection(); auto callee = CreatePeerConnection(); rtc::scoped_refptr audio_track( pc_factory_->CreateAudioTrack("audio_track", nullptr)); auto sender = caller->pc()->AddTrack(audio_track.get(), {}); - ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); - ASSERT_EQ(1u, callee->observer()->add_track_events_.size()); + ASSERT_TRUE( + callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), + static_cast(nullptr))); + ASSERT_EQ(callee->observer()->add_track_events_.size(), 1u); EXPECT_TRUE(caller->pc()->RemoveTrack(sender)); - ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + ASSERT_TRUE( + callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), + static_cast(nullptr))); - ASSERT_EQ(1u, callee->observer()->add_track_events_.size()); + ASSERT_EQ(callee->observer()->add_track_events_.size(), 1u); EXPECT_EQ(callee->observer()->GetAddTrackReceivers(), callee->observer()->remove_track_events_); } -TEST_F(PeerConnectionRtpTest, RemoveTrackWithStreamFiresOnRemoveTrack) { +TEST_F(PeerConnectionRtpCallbacksTest, + RemoveTrackWithStreamFiresOnRemoveTrack) { auto caller = CreatePeerConnection(); auto callee = CreatePeerConnection(); @@ -121,17 +154,22 @@ TEST_F(PeerConnectionRtpTest, RemoveTrackWithStreamFiresOnRemoveTrack) { pc_factory_->CreateAudioTrack("audio_track", nullptr)); auto stream = webrtc::MediaStream::Create("audio_stream"); auto sender = caller->pc()->AddTrack(audio_track.get(), {stream.get()}); - ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); - ASSERT_EQ(1u, callee->observer()->add_track_events_.size()); + ASSERT_TRUE( + callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), + static_cast(nullptr))); + ASSERT_EQ(callee->observer()->add_track_events_.size(), 1u); EXPECT_TRUE(caller->pc()->RemoveTrack(sender)); - ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + ASSERT_TRUE( + callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), + static_cast(nullptr))); - ASSERT_EQ(1u, callee->observer()->add_track_events_.size()); + ASSERT_EQ(callee->observer()->add_track_events_.size(), 1u); EXPECT_EQ(callee->observer()->GetAddTrackReceivers(), callee->observer()->remove_track_events_); } -TEST_F(PeerConnectionRtpTest, RemoveTrackWithSharedStreamFiresOnRemoveTrack) { +TEST_F(PeerConnectionRtpCallbacksTest, + RemoveTrackWithSharedStreamFiresOnRemoveTrack) { auto caller = CreatePeerConnection(); auto callee = CreatePeerConnection(); @@ -143,14 +181,18 @@ TEST_F(PeerConnectionRtpTest, RemoveTrackWithSharedStreamFiresOnRemoveTrack) { std::vector streams{stream.get()}; auto sender1 = caller->pc()->AddTrack(audio_track1.get(), streams); auto sender2 = caller->pc()->AddTrack(audio_track2.get(), streams); - ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + ASSERT_TRUE( + callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), + static_cast(nullptr))); - ASSERT_EQ(2u, callee->observer()->add_track_events_.size()); + ASSERT_EQ(callee->observer()->add_track_events_.size(), 2u); // Remove "audio_track1". EXPECT_TRUE(caller->pc()->RemoveTrack(sender1)); - ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); - ASSERT_EQ(2u, callee->observer()->add_track_events_.size()); + ASSERT_TRUE( + callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), + static_cast(nullptr))); + ASSERT_EQ(callee->observer()->add_track_events_.size(), 2u); EXPECT_EQ( std::vector>{ callee->observer()->add_track_events_[0].receiver}, @@ -158,10 +200,211 @@ TEST_F(PeerConnectionRtpTest, RemoveTrackWithSharedStreamFiresOnRemoveTrack) { // Remove "audio_track2". EXPECT_TRUE(caller->pc()->RemoveTrack(sender2)); - ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); - ASSERT_EQ(2u, callee->observer()->add_track_events_.size()); + ASSERT_TRUE( + callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), + static_cast(nullptr))); + ASSERT_EQ(callee->observer()->add_track_events_.size(), 2u); EXPECT_EQ(callee->observer()->GetAddTrackReceivers(), callee->observer()->remove_track_events_); } +// These tests examine the state of the peer connection as a result of +// performing SetRemoteDescription(). +class PeerConnectionRtpObserverTest : public PeerConnectionRtpTest {}; + +TEST_F(PeerConnectionRtpObserverTest, AddSenderWithoutStreamAddsReceiver) { + auto caller = CreatePeerConnection(); + auto callee = CreatePeerConnection(); + + rtc::scoped_refptr audio_track( + pc_factory_->CreateAudioTrack("audio_track", nullptr)); + EXPECT_TRUE(caller->pc()->AddTrack(audio_track.get(), {})); + ASSERT_TRUE( + callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), + static_cast(nullptr))); + + EXPECT_EQ(callee->pc()->GetReceivers().size(), 1u); + auto receiver_added = callee->pc()->GetReceivers()[0]; + EXPECT_EQ("audio_track", receiver_added->track()->id()); + // TODO(hbos): When "no stream" is handled correctly we would expect + // |receiver_added->streams()| to be empty. https://crbug.com/webrtc/7933 + EXPECT_EQ(receiver_added->streams().size(), 1u); + EXPECT_TRUE(receiver_added->streams()[0]->FindAudioTrack("audio_track")); +} + +TEST_F(PeerConnectionRtpObserverTest, AddSenderWithStreamAddsReceiver) { + auto caller = CreatePeerConnection(); + auto callee = CreatePeerConnection(); + + rtc::scoped_refptr audio_track( + pc_factory_->CreateAudioTrack("audio_track", nullptr)); + auto stream = webrtc::MediaStream::Create("audio_stream"); + EXPECT_TRUE(caller->pc()->AddTrack(audio_track.get(), {stream})); + ASSERT_TRUE( + callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), + static_cast(nullptr))); + + EXPECT_EQ(callee->pc()->GetReceivers().size(), 1u); + auto receiver_added = callee->pc()->GetReceivers()[0]; + EXPECT_EQ("audio_track", receiver_added->track()->id()); + EXPECT_EQ(receiver_added->streams().size(), 1u); + EXPECT_EQ("audio_stream", receiver_added->streams()[0]->label()); + EXPECT_TRUE(receiver_added->streams()[0]->FindAudioTrack("audio_track")); +} + +TEST_F(PeerConnectionRtpObserverTest, + RemoveSenderWithoutStreamRemovesReceiver) { + auto caller = CreatePeerConnection(); + auto callee = CreatePeerConnection(); + + rtc::scoped_refptr audio_track( + pc_factory_->CreateAudioTrack("audio_track", nullptr)); + auto sender = caller->pc()->AddTrack(audio_track.get(), {}); + ASSERT_TRUE(sender); + ASSERT_TRUE( + callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), + static_cast(nullptr))); + ASSERT_EQ(callee->pc()->GetReceivers().size(), 1u); + auto receiver = callee->pc()->GetReceivers()[0]; + ASSERT_TRUE(caller->pc()->RemoveTrack(sender)); + ASSERT_TRUE( + callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), + static_cast(nullptr))); + + // TODO(hbos): When we implement Unified Plan, receivers will not be removed. + // Instead, the transceiver owning the receiver will become inactive. + EXPECT_EQ(callee->pc()->GetReceivers().size(), 0u); +} + +TEST_F(PeerConnectionRtpObserverTest, RemoveSenderWithStreamRemovesReceiver) { + auto caller = CreatePeerConnection(); + auto callee = CreatePeerConnection(); + + rtc::scoped_refptr audio_track( + pc_factory_->CreateAudioTrack("audio_track", nullptr)); + auto stream = webrtc::MediaStream::Create("audio_stream"); + auto sender = caller->pc()->AddTrack(audio_track.get(), {stream}); + ASSERT_TRUE(sender); + ASSERT_TRUE( + callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), + static_cast(nullptr))); + ASSERT_EQ(callee->pc()->GetReceivers().size(), 1u); + auto receiver = callee->pc()->GetReceivers()[0]; + ASSERT_TRUE(caller->pc()->RemoveTrack(sender)); + ASSERT_TRUE( + callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), + static_cast(nullptr))); + + // TODO(hbos): When we implement Unified Plan, receivers will not be removed. + // Instead, the transceiver owning the receiver will become inactive. + EXPECT_EQ(callee->pc()->GetReceivers().size(), 0u); +} + +TEST_F(PeerConnectionRtpObserverTest, + RemoveSenderWithSharedStreamRemovesReceiver) { + auto caller = CreatePeerConnection(); + auto callee = CreatePeerConnection(); + + rtc::scoped_refptr audio_track1( + pc_factory_->CreateAudioTrack("audio_track1", nullptr)); + rtc::scoped_refptr audio_track2( + pc_factory_->CreateAudioTrack("audio_track2", nullptr)); + auto stream = webrtc::MediaStream::Create("shared_audio_stream"); + std::vector streams{stream.get()}; + auto sender1 = caller->pc()->AddTrack(audio_track1.get(), streams); + auto sender2 = caller->pc()->AddTrack(audio_track2.get(), streams); + ASSERT_TRUE( + callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), + static_cast(nullptr))); + + ASSERT_EQ(callee->pc()->GetReceivers().size(), 2u); + rtc::scoped_refptr receiver1; + rtc::scoped_refptr receiver2; + if (callee->pc()->GetReceivers()[0]->track()->id() == "audio_track1") { + receiver1 = callee->pc()->GetReceivers()[0]; + receiver2 = callee->pc()->GetReceivers()[1]; + } else { + receiver1 = callee->pc()->GetReceivers()[1]; + receiver2 = callee->pc()->GetReceivers()[0]; + } + EXPECT_EQ("audio_track1", receiver1->track()->id()); + EXPECT_EQ("audio_track2", receiver2->track()->id()); + + // Remove "audio_track1". + EXPECT_TRUE(caller->pc()->RemoveTrack(sender1)); + ASSERT_TRUE( + callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), + static_cast(nullptr))); + // Only |receiver2| should remain. + // TODO(hbos): When we implement Unified Plan, receivers will not be removed. + // Instead, the transceiver owning the receiver will become inactive. + EXPECT_EQ( + std::vector>{receiver2}, + callee->pc()->GetReceivers()); + + // Remove "audio_track2". + EXPECT_TRUE(caller->pc()->RemoveTrack(sender2)); + ASSERT_TRUE( + callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), + static_cast(nullptr))); + // TODO(hbos): When we implement Unified Plan, receivers will not be removed. + // Instead, the transceiver owning the receiver will become inactive. + EXPECT_EQ(callee->pc()->GetReceivers().size(), 0u); +} + +// Invokes SetRemoteDescription() twice in a row without synchronizing the two +// calls and examine the state of the peer connection inside the callbacks to +// ensure that the second call does not occur prematurely, contaminating the +// state of the peer connection of the first callback. +TEST_F(PeerConnectionRtpObserverTest, + StatesCorrelateWithSetRemoteDescriptionCall) { + auto caller = CreatePeerConnection(); + auto callee = CreatePeerConnection(); + + rtc::scoped_refptr audio_track( + pc_factory_->CreateAudioTrack("audio_track", nullptr)); + // Create SDP for adding a track and for removing it. This will be used in the + // first and second SetRemoteDescription() calls. + auto sender = caller->pc()->AddTrack(audio_track.get(), {}); + auto srd1_sdp = caller->CreateOfferAndSetAsLocal(); + EXPECT_TRUE(caller->pc()->RemoveTrack(sender)); + auto srd2_sdp = caller->CreateOfferAndSetAsLocal(); + + // In the first SetRemoteDescription() callback, check that we have a + // receiver for the track. + auto pc = callee->pc(); + bool srd1_callback_called = false; + auto srd1_callback = [&srd1_callback_called, &pc]() { + EXPECT_EQ(pc->GetReceivers().size(), 1u); + srd1_callback_called = true; + }; + + // In the second SetRemoteDescription() callback, check that the receiver has + // been removed. + // TODO(hbos): When we implement Unified Plan, receivers will not be removed. + // Instead, the transceiver owning the receiver will become inactive. + // https://crbug.com/webrtc/7600 + bool srd2_callback_called = false; + auto srd2_callback = [&srd2_callback_called, &pc]() { + EXPECT_TRUE(pc->GetReceivers().empty()); + srd2_callback_called = true; + }; + + // Invoke SetRemoteDescription() twice in a row without synchronizing the two + // calls. The callbacks verify that the two calls are synchronized, as in, the + // effects of the second SetRemoteDescription() call must not have happened by + // the time the first callback is invoked. If it has then the receiver that is + // added as a result of the first SetRemoteDescription() call will already + // have been removed as a result of the second SetRemoteDescription() call + // when the first callback is invoked. + callee->pc()->SetRemoteDescription( + std::move(srd1_sdp), + new OnSuccessObserver(srd1_callback)); + callee->pc()->SetRemoteDescription( + std::move(srd2_sdp), + new OnSuccessObserver(srd2_callback)); + EXPECT_TRUE_WAIT(srd1_callback_called, kDefaultTimeout); + EXPECT_TRUE_WAIT(srd2_callback_called, kDefaultTimeout); +} + } // namespace diff --git a/pc/peerconnectionwrapper.cc b/pc/peerconnectionwrapper.cc index 14308b5789..1b92efaac3 100644 --- a/pc/peerconnectionwrapper.cc +++ b/pc/peerconnectionwrapper.cc @@ -153,6 +153,19 @@ bool PeerConnectionWrapper::SetRemoteDescription( error_out); } +bool PeerConnectionWrapper::SetRemoteDescription( + std::unique_ptr desc, + RTCError* error_out) { + rtc::scoped_refptr observer = + new MockSetRemoteDescriptionObserver(); + pc()->SetRemoteDescription(std::move(desc), observer); + EXPECT_EQ_WAIT(true, observer->called(), kDefaultTimeout); + bool ok = observer->error().ok(); + if (error_out) + *error_out = std::move(observer->error()); + return ok; +} + bool PeerConnectionWrapper::SetSdp( rtc::FunctionView fn, std::string* error_out) { diff --git a/pc/peerconnectionwrapper.h b/pc/peerconnectionwrapper.h index ca805a3c13..8918a712f1 100644 --- a/pc/peerconnectionwrapper.h +++ b/pc/peerconnectionwrapper.h @@ -90,6 +90,8 @@ class PeerConnectionWrapper { // Returns true if the description was successfully set. bool SetRemoteDescription(std::unique_ptr desc, std::string* error_out = nullptr); + bool SetRemoteDescription(std::unique_ptr desc, + RTCError* error_out); // Calls the underlying PeerConnection's AddTrack method with an audio media // stream track not bound to any source. diff --git a/pc/setremotedescriptionobserverinterface_unittest.cc b/pc/setremotedescriptionobserverinterface_unittest.cc new file mode 100644 index 0000000000..676195f15b --- /dev/null +++ b/pc/setremotedescriptionobserverinterface_unittest.cc @@ -0,0 +1,75 @@ +/* + * Copyright 2017 The WebRTC project authors. All Rights Reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +#include "api/setremotedescriptionobserverinterface.h" + +#include + +#include "api/jsep.h" +#include "api/optional.h" +#include "api/rtcerror.h" +#include "pc/test/mockpeerconnectionobservers.h" +#include "rtc_base/checks.h" +#include "rtc_base/gunit.h" +#include "rtc_base/scoped_ref_ptr.h" +#include "rtc_base/thread.h" + +// TODO(hbos): This is a test for api/setremotedescriptionobserverinterface.h +// and should be in api/ instead of pc/, but the dependency on +// pc/test/mockpeerconnectionobservers.h and rtc_base/thread.h is not allowed +// from api:rtc_api_unittests. + +const int kDefaultTimeoutMs = 1000; + +class SetRemoteDescriptionObserverWrapperTest : public testing::Test { + public: + SetRemoteDescriptionObserverWrapperTest() + : set_desc_observer_(new rtc::RefCountedObject< + webrtc::MockSetSessionDescriptionObserver>()), + observer_(new webrtc::SetRemoteDescriptionObserverAdapter( + set_desc_observer_)) {} + + protected: + rtc::scoped_refptr + set_desc_observer_; + rtc::scoped_refptr observer_; +}; + +TEST_F(SetRemoteDescriptionObserverWrapperTest, OnCompleteWithSuccess) { + observer_->OnSetRemoteDescriptionComplete(webrtc::RTCError::OK()); + EXPECT_TRUE_WAIT(set_desc_observer_->called(), kDefaultTimeoutMs); + EXPECT_TRUE(set_desc_observer_->result()); +} + +TEST_F(SetRemoteDescriptionObserverWrapperTest, OnCompleteWithFailure) { + observer_->OnSetRemoteDescriptionComplete(webrtc::RTCError( + webrtc::RTCErrorType::INVALID_PARAMETER, "FailureMessage")); + EXPECT_TRUE_WAIT(set_desc_observer_->called(), kDefaultTimeoutMs); + EXPECT_FALSE(set_desc_observer_->result()); + EXPECT_EQ(set_desc_observer_->error(), "FailureMessage"); +} + +TEST_F(SetRemoteDescriptionObserverWrapperTest, IsAsynchronous) { + observer_->OnSetRemoteDescriptionComplete(webrtc::RTCError::OK()); + // Untill this thread's messages are processed by EXPECT_TRUE_WAIT, + // |set_desc_observer_| should not have been called. + EXPECT_FALSE(set_desc_observer_->called()); + EXPECT_TRUE_WAIT(set_desc_observer_->called(), kDefaultTimeoutMs); + EXPECT_TRUE(set_desc_observer_->result()); +} + +TEST_F(SetRemoteDescriptionObserverWrapperTest, SurvivesDereferencing) { + observer_->OnSetRemoteDescriptionComplete(webrtc::RTCError::OK()); + // Even if there are no external references to |observer_| the operation + // should complete. + observer_ = nullptr; + EXPECT_TRUE_WAIT(set_desc_observer_->called(), kDefaultTimeoutMs); + EXPECT_TRUE(set_desc_observer_->result()); +} diff --git a/pc/test/mockpeerconnectionobservers.h b/pc/test/mockpeerconnectionobservers.h index 9128b8c378..b45c67d657 100644 --- a/pc/test/mockpeerconnectionobservers.h +++ b/pc/test/mockpeerconnectionobservers.h @@ -213,12 +213,12 @@ class MockSetSessionDescriptionObserver MockSetSessionDescriptionObserver() : called_(false), error_("MockSetSessionDescriptionObserver not called") {} - virtual ~MockSetSessionDescriptionObserver() {} - virtual void OnSuccess() { + ~MockSetSessionDescriptionObserver() override {} + void OnSuccess() override { called_ = true; error_ = ""; } - virtual void OnFailure(const std::string& error) { + void OnFailure(const std::string& error) override { called_ = true; error_ = error; } @@ -231,6 +231,25 @@ class MockSetSessionDescriptionObserver std::string error_; }; +class MockSetRemoteDescriptionObserver + : public rtc::RefCountedObject { + public: + bool called() const { return error_.has_value(); } + RTCError& error() { + RTC_DCHECK(error_.has_value()); + return *error_; + } + + // SetRemoteDescriptionObserverInterface implementation. + void OnSetRemoteDescriptionComplete(RTCError error) override { + error_ = std::move(error); + } + + private: + // Set on complete, on success this is set to an RTCError::OK() error. + rtc::Optional error_; +}; + class MockDataChannelObserver : public webrtc::DataChannelObserver { public: explicit MockDataChannelObserver(webrtc::DataChannelInterface* channel)