From 6c7ec32bd63ab2b45d4d83ae1de817ee946b4d72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= Date: Wed, 22 Nov 2017 17:43:47 +0100 Subject: [PATCH] SetRemoteDescriptionObserverInterface added. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The new observer replaced SetSessionDescriptionObserver for SetRemoteDescription. Unlike SetSessionDescriptionObserver, SetRemoteDescriptionObserverInterface is invoked synchronously so that the you can rely on the state of the PeerConnection to represent the result of the SetRemoteDescription call in the callback. The new observer succeeds or fails with an RTCError. This deprecates the need for PeerConnectionObserver::OnAdd/RemoveTrack and SetSessionDescriptionObserver, with the benefit that all media object changes can be processed in a single callback by the application in a synchronous callback. This will help Chromium keep objects in-sync across layers and threads in a non-racy and straight-forward way, see design doc (Proposal 2): https://docs.google.com/a/google.com/document/d/1-cDDC82mgU5zrHacfFz720p3xwRtuBkOPSRchh07Ho0/edit?usp=sharing An adapter for SetSessionDescriptionObserver is added to allow calling the old SetRemoteDescription signature and get the old behavior (OnSuccess/OnFailure callback in a Post) until third parties switch. Bug: webrtc:8473 Change-Id: I3d4eb60da6dd34615f2c9f384aeaf4634e648c99 Reviewed-on: https://webrtc-review.googlesource.com/17523 Commit-Queue: Henrik Boström Reviewed-by: Peter Thatcher Reviewed-by: Guido Urdaneta Cr-Commit-Position: refs/heads/master@{#20841} --- api/BUILD.gn | 3 + api/peerconnectioninterface.h | 13 +- api/peerconnectionproxy.h | 4 + api/setremotedescriptionobserverinterface.cc | 61 ++++ api/setremotedescriptionobserverinterface.h | 69 ++++ pc/BUILD.gn | 2 + pc/peerconnection.cc | 34 +- pc/peerconnection.h | 19 +- pc/peerconnection_rtp_unittest.cc | 295 ++++++++++++++++-- pc/peerconnectionwrapper.cc | 13 + pc/peerconnectionwrapper.h | 2 + ...tedescriptionobserverinterface_unittest.cc | 75 +++++ pc/test/mockpeerconnectionobservers.h | 25 +- 13 files changed, 561 insertions(+), 54 deletions(-) create mode 100644 api/setremotedescriptionobserverinterface.cc create mode 100644 api/setremotedescriptionobserverinterface.h create mode 100644 pc/setremotedescriptionobserverinterface_unittest.cc 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)