Revert "SetRemoteDescriptionObserverInterface added."
This reverts commit 6c7ec32bd63ab2b45d4d83ae1de817ee946b4d72.
Reason for revert: Third party project breaks due to use-after-free
in the callback. I suspect this is because the adapter is processing
the async callback instead of the pc, i.e. callback is called from
SetRemoteDescriptionObserverAdapter::OnMessage instead of from
PeerConnection::OnMessage. This makes it possible for the callback to
be invoked after the PC is destroyed.
I argue this is how it should be done, and that if you're using a raw
pointer in an async callback you're doing it wrong, but I will reland
this CL with the callback processed in PeerConnection::OnMessage
instead as to not change the behavior of the old SRD signature.
Original change's description:
> SetRemoteDescriptionObserverInterface added.
>
> 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 <hbos@webrtc.org>
> Reviewed-by: Peter Thatcher <pthatcher@webrtc.org>
> Reviewed-by: Guido Urdaneta <guidou@webrtc.org>
> Cr-Commit-Position: refs/heads/master@{#20841}
TBR=hbos@webrtc.org,hta@webrtc.org,pthatcher@webrtc.org,guidou@webrtc.org
Change-Id: I715555e084d9aae49ee2a8831c70dc006dbdb74c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: webrtc:8473
Reviewed-on: https://webrtc-review.googlesource.com/25580
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#20850}diff --git a/api/BUILD.gn b/api/BUILD.gn
index 278ab15..03460f6 100644
--- a/api/BUILD.gn
+++ b/api/BUILD.gn
@@ -63,8 +63,6 @@
"rtpreceiverinterface.h",
"rtpsenderinterface.h",
"rtptransceiverinterface.h",
- "setremotedescriptionobserverinterface.cc",
- "setremotedescriptionobserverinterface.h",
"statstypes.cc",
"statstypes.h",
"turncustomizer.h",
@@ -381,7 +379,6 @@
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 0cff5df..a682459 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 @@
// 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<SessionDescriptionInterface>(desc),
- rtc::scoped_refptr<SetRemoteDescriptionObserverInterface>(
- new SetRemoteDescriptionObserverAdapter(observer)));
- }
- // TODO(hbos): Make pure virtual when Chrome has updated its signature.
- virtual void SetRemoteDescription(
- std::unique_ptr<SessionDescriptionInterface> desc,
- rtc::scoped_refptr<SetRemoteDescriptionObserverInterface> 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 bdd9fcb..36ee50f 100644
--- a/api/peerconnectionproxy.h
+++ b/api/peerconnectionproxy.h
@@ -88,10 +88,6 @@
SetRemoteDescription,
SetSessionDescriptionObserver*,
SessionDescriptionInterface*)
- PROXY_METHOD2(void,
- SetRemoteDescription,
- std::unique_ptr<SessionDescriptionInterface>,
- rtc::scoped_refptr<SetRemoteDescriptionObserverInterface>);
PROXY_METHOD0(PeerConnectionInterface::RTCConfiguration, GetConfiguration);
PROXY_METHOD2(bool,
SetConfiguration,
diff --git a/api/setremotedescriptionobserverinterface.cc b/api/setremotedescriptionobserverinterface.cc
deleted file mode 100644
index 67006a3..0000000
--- 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 <string>
-
-namespace webrtc {
-
-// The message keeps the observer alive through reference counting.
-class SetRemoteDescriptionObserverAdapter::MessageData
- : public rtc::MessageData {
- public:
- static MessageData* Create(
- rtc::scoped_refptr<SetRemoteDescriptionObserverAdapter> observer,
- RTCError error) {
- return new MessageData(std::move(observer), std::move(error));
- }
-
- const RTCError& error() const { return error_; }
-
- private:
- MessageData(rtc::scoped_refptr<SetRemoteDescriptionObserverAdapter> observer,
- RTCError error)
- : observer_(std::move(observer)), error_(std::move(error)) {}
-
- rtc::scoped_refptr<SetRemoteDescriptionObserverAdapter> observer_;
- RTCError error_;
-};
-
-SetRemoteDescriptionObserverAdapter::SetRemoteDescriptionObserverAdapter(
- rtc::scoped_refptr<SetSessionDescriptionObserver> 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<MessageData*>(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 f1b65a1..0000000
--- 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 <utility>
-#include <vector>
-
-#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<SetRemoteDescriptionObserverInterface>,
- public rtc::MessageHandler {
- public:
- SetRemoteDescriptionObserverAdapter(
- rtc::scoped_refptr<SetSessionDescriptionObserver> wrapper);
-
- // SetRemoteDescriptionObserverInterface implementation.
- void OnSetRemoteDescriptionComplete(RTCError error) override;
-
- // rtc::MessageHandler implementation.
- void OnMessage(rtc::Message* msg) override;
-
- private:
- class MessageData;
-
- rtc::scoped_refptr<SetSessionDescriptionObserver> wrapper_;
-};
-
-} // namespace webrtc
-
-#endif // API_SETREMOTEDESCRIPTIONOBSERVERINTERFACE_H_
diff --git a/pc/BUILD.gn b/pc/BUILD.gn
index 1c524d2..d4ee2a5 100644
--- a/pc/BUILD.gn
+++ b/pc/BUILD.gn
@@ -402,7 +402,6 @@
"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 @@
"..: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 08370d9..62e0ef8 100644
--- a/pc/peerconnection.cc
+++ b/pc/peerconnection.cc
@@ -1463,7 +1463,7 @@
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::SetRemoteDescription(
- std::unique_ptr<SessionDescriptionInterface> desc,
- rtc::scoped_refptr<SetRemoteDescriptionObserverInterface> 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<SessionDescriptionInterface> 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 @@
// 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 @@
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 @@
role);
}
-bool PeerConnection::SetCurrentOrPendingLocalDescription(
+bool PeerConnection::SetLocalDescription(
std::unique_ptr<SessionDescriptionInterface> desc,
std::string* err_desc) {
RTC_DCHECK(signaling_thread()->IsCurrent());
@@ -3314,7 +3316,7 @@
return true;
}
-bool PeerConnection::SetCurrentOrPendingRemoteDescription(
+bool PeerConnection::SetRemoteDescription(
std::unique_ptr<SessionDescriptionInterface> desc,
std::string* err_desc) {
RTC_DCHECK(signaling_thread()->IsCurrent());
diff --git a/pc/peerconnection.h b/pc/peerconnection.h
index a67780b..2b36a86 100644
--- a/pc/peerconnection.h
+++ b/pc/peerconnection.h
@@ -150,10 +150,8 @@
const RTCOfferAnswerOptions& options) override;
void SetLocalDescription(SetSessionDescriptionObserver* observer,
SessionDescriptionInterface* desc) override;
- void SetRemoteDescription(
- std::unique_ptr<SessionDescriptionInterface> desc,
- rtc::scoped_refptr<SetRemoteDescriptionObserverInterface> observer)
- override;
+ void SetRemoteDescription(SetSessionDescriptionObserver* observer,
+ SessionDescriptionInterface* desc) override;
PeerConnectionInterface::RTCConfiguration GetConfiguration() override;
bool SetConfiguration(
const PeerConnectionInterface::RTCConfiguration& configuration,
@@ -541,15 +539,10 @@
// 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<SessionDescriptionInterface> desc,
- std::string* err_desc);
- bool SetCurrentOrPendingRemoteDescription(
- std::unique_ptr<SessionDescriptionInterface> desc,
- std::string* err_desc);
+ bool SetLocalDescription(std::unique_ptr<SessionDescriptionInterface> desc,
+ std::string* err_desc);
+ bool SetRemoteDescription(std::unique_ptr<SessionDescriptionInterface> 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 ae7b67d..860c6dc 100644
--- a/pc/peerconnection_rtp_unittest.cc
+++ b/pc/peerconnection_rtp_unittest.cc
@@ -33,25 +33,6 @@
namespace {
-const uint32_t kDefaultTimeout = 10000u;
-
-template <typename MethodFunctor>
-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 @@
rtc::scoped_refptr<webrtc::PeerConnectionFactoryInterface> 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<webrtc::AudioTrackInterface> audio_track(
pc_factory_->CreateAudioTrack("audio_track", nullptr));
EXPECT_TRUE(caller->pc()->AddTrack(audio_track.get(), {}));
- ASSERT_TRUE(
- callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
- static_cast<webrtc::RTCError*>(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 @@
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<webrtc::RTCError*>(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<webrtc::AudioTrackInterface> audio_track(
pc_factory_->CreateAudioTrack("audio_track", nullptr));
auto sender = caller->pc()->AddTrack(audio_track.get(), {});
- ASSERT_TRUE(
- callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
- static_cast<webrtc::RTCError*>(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<webrtc::RTCError*>(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 @@
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<webrtc::RTCError*>(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<webrtc::RTCError*>(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 @@
std::vector<webrtc::MediaStreamInterface*> 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<webrtc::RTCError*>(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<webrtc::RTCError*>(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<rtc::scoped_refptr<webrtc::RtpReceiverInterface>>{
callee->observer()->add_track_events_[0].receiver},
@@ -200,211 +158,10 @@
// Remove "audio_track2".
EXPECT_TRUE(caller->pc()->RemoveTrack(sender2));
- ASSERT_TRUE(
- callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
- static_cast<webrtc::RTCError*>(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<webrtc::AudioTrackInterface> audio_track(
- pc_factory_->CreateAudioTrack("audio_track", nullptr));
- EXPECT_TRUE(caller->pc()->AddTrack(audio_track.get(), {}));
- ASSERT_TRUE(
- callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
- static_cast<webrtc::RTCError*>(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<webrtc::AudioTrackInterface> 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<webrtc::RTCError*>(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<webrtc::AudioTrackInterface> 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<webrtc::RTCError*>(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<webrtc::RTCError*>(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<webrtc::AudioTrackInterface> 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<webrtc::RTCError*>(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<webrtc::RTCError*>(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<webrtc::AudioTrackInterface> audio_track1(
- pc_factory_->CreateAudioTrack("audio_track1", nullptr));
- rtc::scoped_refptr<webrtc::AudioTrackInterface> audio_track2(
- pc_factory_->CreateAudioTrack("audio_track2", nullptr));
- auto stream = webrtc::MediaStream::Create("shared_audio_stream");
- std::vector<webrtc::MediaStreamInterface*> 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<webrtc::RTCError*>(nullptr)));
-
- ASSERT_EQ(callee->pc()->GetReceivers().size(), 2u);
- rtc::scoped_refptr<webrtc::RtpReceiverInterface> receiver1;
- rtc::scoped_refptr<webrtc::RtpReceiverInterface> 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<webrtc::RTCError*>(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<rtc::scoped_refptr<webrtc::RtpReceiverInterface>>{receiver2},
- callee->pc()->GetReceivers());
-
- // Remove "audio_track2".
- EXPECT_TRUE(caller->pc()->RemoveTrack(sender2));
- ASSERT_TRUE(
- callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
- static_cast<webrtc::RTCError*>(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<webrtc::AudioTrackInterface> 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<decltype(srd1_callback)>(srd1_callback));
- callee->pc()->SetRemoteDescription(
- std::move(srd2_sdp),
- new OnSuccessObserver<decltype(srd2_callback)>(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 1b92efa..14308b5 100644
--- a/pc/peerconnectionwrapper.cc
+++ b/pc/peerconnectionwrapper.cc
@@ -153,19 +153,6 @@
error_out);
}
-bool PeerConnectionWrapper::SetRemoteDescription(
- std::unique_ptr<SessionDescriptionInterface> desc,
- RTCError* error_out) {
- rtc::scoped_refptr<MockSetRemoteDescriptionObserver> 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<void(SetSessionDescriptionObserver*)> fn,
std::string* error_out) {
diff --git a/pc/peerconnectionwrapper.h b/pc/peerconnectionwrapper.h
index 8918a71..ca805a3 100644
--- a/pc/peerconnectionwrapper.h
+++ b/pc/peerconnectionwrapper.h
@@ -90,8 +90,6 @@
// Returns true if the description was successfully set.
bool SetRemoteDescription(std::unique_ptr<SessionDescriptionInterface> desc,
std::string* error_out = nullptr);
- bool SetRemoteDescription(std::unique_ptr<SessionDescriptionInterface> 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 676195f..0000000
--- 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 <string>
-
-#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<webrtc::MockSetSessionDescriptionObserver>
- set_desc_observer_;
- rtc::scoped_refptr<webrtc::SetRemoteDescriptionObserverAdapter> 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 b45c67d..9128b8c 100644
--- a/pc/test/mockpeerconnectionobservers.h
+++ b/pc/test/mockpeerconnectionobservers.h
@@ -213,12 +213,12 @@
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 @@
std::string error_;
};
-class MockSetRemoteDescriptionObserver
- : public rtc::RefCountedObject<SetRemoteDescriptionObserverInterface> {
- 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<RTCError> error_;
-};
-
class MockDataChannelObserver : public webrtc::DataChannelObserver {
public:
explicit MockDataChannelObserver(webrtc::DataChannelInterface* channel)
