diff --git a/api/BUILD.gn b/api/BUILD.gn index 278ab1536e..03460f6e70 100644 --- a/api/BUILD.gn +++ b/api/BUILD.gn @@ -63,8 +63,6 @@ rtc_static_library("libjingle_peerconnection_api") { "rtpreceiverinterface.h", "rtpsenderinterface.h", "rtptransceiverinterface.h", - "setremotedescriptionobserverinterface.cc", - "setremotedescriptionobserverinterface.h", "statstypes.cc", "statstypes.h", "turncustomizer.h", @@ -381,7 +379,6 @@ 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 0cff5df2d8..a682459a13 100644 --- a/api/peerconnectioninterface.h +++ b/api/peerconnectioninterface.h @@ -82,7 +82,6 @@ #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" @@ -727,18 +726,8 @@ 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) { - 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) {} + SessionDescriptionInterface* desc) = 0; // 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 bdd9fcb16d..36ee50f9d0 100644 --- a/api/peerconnectionproxy.h +++ b/api/peerconnectionproxy.h @@ -88,10 +88,6 @@ 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 deleted file mode 100644 index 67006a3900..0000000000 --- a/api/setremotedescriptionobserverinterface.cc +++ /dev/null @@ -1,61 +0,0 @@ -/* - * 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 deleted file mode 100644 index f1b65a1f1f..0000000000 --- a/api/setremotedescriptionobserverinterface.h +++ /dev/null @@ -1,69 +0,0 @@ -/* - * 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 1c524d299d..d4ee2a533a 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn @@ -402,7 +402,6 @@ 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", @@ -444,7 +443,6 @@ 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 08370d9f61..62e0ef868f 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 (!SetCurrentOrPendingLocalDescription(std::move(desc_temp), &error)) { + if (!SetLocalDescription(std::move(desc_temp), &error)) { PostSetSessionDescriptionFailure(observer, error); return; } @@ -1541,25 +1541,26 @@ void PeerConnection::SetLocalDescription( } void PeerConnection::SetRemoteDescription( - std::unique_ptr desc, - rtc::scoped_refptr observer) { + SetSessionDescriptionObserver* observer, + SessionDescriptionInterface* desc) { TRACE_EVENT0("webrtc", "PeerConnection::SetRemoteDescription"); if (!observer) { RTC_LOG(LS_ERROR) << "SetRemoteDescription - observer is NULL."; return; } if (!desc) { - observer->OnSetRemoteDescriptionComplete(RTCError( - RTCErrorType::UNSUPPORTED_PARAMETER, "SessionDescription is NULL.")); + PostSetSessionDescriptionFailure(observer, "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->type() + + std::string error = "Failed to set remote " + desc_temp->type() + " sdp: Called in wrong state: STATE_CLOSED"; RTC_LOG(LS_ERROR) << error; - observer->OnSetRemoteDescriptionComplete( - RTCError(RTCErrorType::INVALID_STATE, std::move(error))); + PostSetSessionDescriptionFailure(observer, error); return; } @@ -1567,11 +1568,10 @@ void PeerConnection::SetRemoteDescription( // streams that might be removed by updating the session description. stats_->UpdateStats(kStatsOutputLevelStandard); std::string 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))); + // 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); return; } RTC_DCHECK(remote_description()); @@ -1661,7 +1661,9 @@ void PeerConnection::SetRemoteDescription( UpdateEndedRemoteMediaStreams(); - observer->OnSetRemoteDescriptionComplete(RTCError::OK()); + SetSessionDescriptionMsg* msg = new SetSessionDescriptionMsg(observer); + signaling_thread()->Post(RTC_FROM_HERE, this, + MSG_SET_SESSIONDESCRIPTION_SUCCESS, msg); if (remote_description()->type() == SessionDescriptionInterface::kAnswer) { // TODO(deadbeef): We already had to hop to the network thread for @@ -3258,7 +3260,7 @@ bool PeerConnection::GetSslRole(const std::string& content_name, role); } -bool PeerConnection::SetCurrentOrPendingLocalDescription( +bool PeerConnection::SetLocalDescription( std::unique_ptr desc, std::string* err_desc) { RTC_DCHECK(signaling_thread()->IsCurrent()); @@ -3314,7 +3316,7 @@ bool PeerConnection::SetCurrentOrPendingLocalDescription( return true; } -bool PeerConnection::SetCurrentOrPendingRemoteDescription( +bool PeerConnection::SetRemoteDescription( std::unique_ptr desc, std::string* err_desc) { RTC_DCHECK(signaling_thread()->IsCurrent()); diff --git a/pc/peerconnection.h b/pc/peerconnection.h index a67780b796..2b36a86e49 100644 --- a/pc/peerconnection.h +++ b/pc/peerconnection.h @@ -150,10 +150,8 @@ class PeerConnection : public PeerConnectionInterface, const RTCOfferAnswerOptions& options) override; void SetLocalDescription(SetSessionDescriptionObserver* observer, SessionDescriptionInterface* desc) override; - void SetRemoteDescription( - std::unique_ptr desc, - rtc::scoped_refptr observer) - override; + void SetRemoteDescription(SetSessionDescriptionObserver* observer, + SessionDescriptionInterface* desc) override; PeerConnectionInterface::RTCConfiguration GetConfiguration() override; bool SetConfiguration( const PeerConnectionInterface::RTCConfiguration& configuration, @@ -541,15 +539,10 @@ class PeerConnection : public PeerConnectionInterface, // Get current SSL role used by SCTP's underlying transport. bool GetSctpSslRole(rtc::SSLRole* role); - // 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); + bool SetLocalDescription(std::unique_ptr desc, + std::string* err_desc); + bool SetRemoteDescription(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 ae7b67d690..860c6dc50e 100644 --- a/pc/peerconnection_rtp_unittest.cc +++ b/pc/peerconnection_rtp_unittest.cc @@ -33,25 +33,6 @@ 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() @@ -79,31 +60,25 @@ class PeerConnectionRtpTest : public testing::Test { rtc::scoped_refptr pc_factory_; }; -// These tests cover |webrtc::PeerConnectionObserver| callbacks firing upon -// setting the remote description. -class PeerConnectionRtpCallbacksTest : public PeerConnectionRtpTest {}; - -TEST_F(PeerConnectionRtpCallbacksTest, AddTrackWithoutStreamFiresOnAddTrack) { +TEST_F(PeerConnectionRtpTest, 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(), - static_cast(nullptr))); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); - ASSERT_EQ(callee->observer()->add_track_events_.size(), 1u); - // TODO(hbos): When "no stream" is handled correctly we would expect + ASSERT_EQ(1u, callee->observer()->add_track_events_.size()); + // TODO(deadbeef): 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(add_track_event.streams.size(), 1u); + ASSERT_EQ(1u, add_track_event.streams.size()); EXPECT_TRUE(add_track_event.streams[0]->FindAudioTrack("audio_track")); EXPECT_EQ(add_track_event.streams, add_track_event.receiver->streams()); } -TEST_F(PeerConnectionRtpCallbacksTest, AddTrackWithStreamFiresOnAddTrack) { +TEST_F(PeerConnectionRtpTest, AddTrackWithStreamFiresOnAddTrack) { auto caller = CreatePeerConnection(); auto callee = CreatePeerConnection(); @@ -111,42 +86,34 @@ TEST_F(PeerConnectionRtpCallbacksTest, 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(), - static_cast(nullptr))); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); - ASSERT_EQ(callee->observer()->add_track_events_.size(), 1u); + ASSERT_EQ(1u, callee->observer()->add_track_events_.size()); auto& add_track_event = callee->observer()->add_track_events_[0]; - ASSERT_EQ(add_track_event.streams.size(), 1u); + ASSERT_EQ(1u, add_track_event.streams.size()); 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(PeerConnectionRtpCallbacksTest, - RemoveTrackWithoutStreamFiresOnRemoveTrack) { +TEST_F(PeerConnectionRtpTest, 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(), - static_cast(nullptr))); - ASSERT_EQ(callee->observer()->add_track_events_.size(), 1u); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + ASSERT_EQ(1u, callee->observer()->add_track_events_.size()); EXPECT_TRUE(caller->pc()->RemoveTrack(sender)); - ASSERT_TRUE( - callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), - static_cast(nullptr))); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); - ASSERT_EQ(callee->observer()->add_track_events_.size(), 1u); + ASSERT_EQ(1u, callee->observer()->add_track_events_.size()); EXPECT_EQ(callee->observer()->GetAddTrackReceivers(), callee->observer()->remove_track_events_); } -TEST_F(PeerConnectionRtpCallbacksTest, - RemoveTrackWithStreamFiresOnRemoveTrack) { +TEST_F(PeerConnectionRtpTest, RemoveTrackWithStreamFiresOnRemoveTrack) { auto caller = CreatePeerConnection(); auto callee = CreatePeerConnection(); @@ -154,22 +121,17 @@ TEST_F(PeerConnectionRtpCallbacksTest, 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(), - static_cast(nullptr))); - ASSERT_EQ(callee->observer()->add_track_events_.size(), 1u); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + ASSERT_EQ(1u, callee->observer()->add_track_events_.size()); EXPECT_TRUE(caller->pc()->RemoveTrack(sender)); - ASSERT_TRUE( - callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), - static_cast(nullptr))); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); - ASSERT_EQ(callee->observer()->add_track_events_.size(), 1u); + ASSERT_EQ(1u, callee->observer()->add_track_events_.size()); EXPECT_EQ(callee->observer()->GetAddTrackReceivers(), callee->observer()->remove_track_events_); } -TEST_F(PeerConnectionRtpCallbacksTest, - RemoveTrackWithSharedStreamFiresOnRemoveTrack) { +TEST_F(PeerConnectionRtpTest, RemoveTrackWithSharedStreamFiresOnRemoveTrack) { auto caller = CreatePeerConnection(); auto callee = CreatePeerConnection(); @@ -181,18 +143,14 @@ TEST_F(PeerConnectionRtpCallbacksTest, 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_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); - ASSERT_EQ(callee->observer()->add_track_events_.size(), 2u); + ASSERT_EQ(2u, callee->observer()->add_track_events_.size()); // Remove "audio_track1". EXPECT_TRUE(caller->pc()->RemoveTrack(sender1)); - ASSERT_TRUE( - callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), - static_cast(nullptr))); - ASSERT_EQ(callee->observer()->add_track_events_.size(), 2u); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + ASSERT_EQ(2u, callee->observer()->add_track_events_.size()); EXPECT_EQ( std::vector>{ callee->observer()->add_track_events_[0].receiver}, @@ -200,211 +158,10 @@ TEST_F(PeerConnectionRtpCallbacksTest, // Remove "audio_track2". EXPECT_TRUE(caller->pc()->RemoveTrack(sender2)); - ASSERT_TRUE( - callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), - static_cast(nullptr))); - ASSERT_EQ(callee->observer()->add_track_events_.size(), 2u); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + ASSERT_EQ(2u, callee->observer()->add_track_events_.size()); 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 1b92efaac3..14308b5789 100644 --- a/pc/peerconnectionwrapper.cc +++ b/pc/peerconnectionwrapper.cc @@ -153,19 +153,6 @@ 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 8918a712f1..ca805a3c13 100644 --- a/pc/peerconnectionwrapper.h +++ b/pc/peerconnectionwrapper.h @@ -90,8 +90,6 @@ 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 deleted file mode 100644 index 676195f15b..0000000000 --- a/pc/setremotedescriptionobserverinterface_unittest.cc +++ /dev/null @@ -1,75 +0,0 @@ -/* - * 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 b45c67d657..9128b8c378 100644 --- a/pc/test/mockpeerconnectionobservers.h +++ b/pc/test/mockpeerconnectionobservers.h @@ -213,12 +213,12 @@ class MockSetSessionDescriptionObserver MockSetSessionDescriptionObserver() : called_(false), error_("MockSetSessionDescriptionObserver not called") {} - ~MockSetSessionDescriptionObserver() override {} - void OnSuccess() override { + virtual ~MockSetSessionDescriptionObserver() {} + virtual void OnSuccess() { called_ = true; error_ = ""; } - void OnFailure(const std::string& error) override { + virtual void OnFailure(const std::string& error) { called_ = true; error_ = error; } @@ -231,25 +231,6 @@ 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)