Reland "SetRemoteDescriptionObserverInterface added."
Description for changes from the original CL:
Calling legacy SRD, implemented using
SetRemoteDescriptionObserverAdapter wrapping the old observer, was
meant to have the exact same behavior as the legacy SRD implementation
which invokes the callbacks in a Post.
However, in the CL that landed and got reverted (PS1), the Adapter had
its own message handler, and callbacks would be invoked even if the PC
was destroyed.
In PS2 I've changed the Adapter to use the PeerConnection's message
handler. If the PC is destroyed, the callback will not be invoked.
This gives identical behavior to before this CL, and the legacy
behavior is covered by a new unittest.
I changed the adapter to be an implementation detail of
peerconnection.cc, therefor some stuff was moved, and the only tests
covering this is now in peerconnection_rtp_unittest.cc.
This is a reland of 6c7ec32bd63ab2b45d4d83ae1de817ee946b4d72
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=pthatcher@webrtc.org
Bug: webrtc:8473
Change-Id: If2b1a1929663b0e77fcc9c2ebeef043e6f73adf5
Reviewed-on: https://webrtc-review.googlesource.com/25640
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Guido Urdaneta <guidou@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#20854}diff --git a/api/BUILD.gn b/api/BUILD.gn
index 03460f6..aa5db93 100644
--- a/api/BUILD.gn
+++ b/api/BUILD.gn
@@ -63,6 +63,7 @@
"rtpreceiverinterface.h",
"rtpsenderinterface.h",
"rtptransceiverinterface.h",
+ "setremotedescriptionobserverinterface.h",
"statstypes.cc",
"statstypes.h",
"turncustomizer.h",
@@ -379,6 +380,7 @@
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 a682459..438e9c1 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,13 @@
// 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;
+ // TODO(hbos): Make pure virtual when Chrome has updated its signature.
+ virtual void SetRemoteDescription(
+ std::unique_ptr<SessionDescriptionInterface> desc,
+ rtc::scoped_refptr<SetRemoteDescriptionObserverInterface> 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 36ee50f..bdd9fcb 100644
--- a/api/peerconnectionproxy.h
+++ b/api/peerconnectionproxy.h
@@ -88,6 +88,10 @@
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.h b/api/setremotedescriptionobserverinterface.h
new file mode 100644
index 0000000..bea8b82
--- /dev/null
+++ b/api/setremotedescriptionobserverinterface.h
@@ -0,0 +1,31 @@
+/*
+ * 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 "api/rtcerror.h"
+#include "rtc_base/refcount.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;
+};
+
+} // namespace webrtc
+
+#endif // API_SETREMOTEDESCRIPTIONOBSERVERINTERFACE_H_
diff --git a/pc/BUILD.gn b/pc/BUILD.gn
index d4ee2a5..da30976 100644
--- a/pc/BUILD.gn
+++ b/pc/BUILD.gn
@@ -443,6 +443,7 @@
"..: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 62e0ef8..65eb603 100644
--- a/pc/peerconnection.cc
+++ b/pc/peerconnection.cc
@@ -641,6 +641,35 @@
} // namespace
+// 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 PeerConnection::SetRemoteDescriptionObserverAdapter
+ : public rtc::RefCountedObject<SetRemoteDescriptionObserverInterface> {
+ public:
+ SetRemoteDescriptionObserverAdapter(
+ rtc::scoped_refptr<PeerConnection> pc,
+ rtc::scoped_refptr<SetSessionDescriptionObserver> wrapper)
+ : pc_(std::move(pc)), wrapper_(std::move(wrapper)) {}
+
+ // SetRemoteDescriptionObserverInterface implementation.
+ void OnSetRemoteDescriptionComplete(RTCError error) override {
+ if (error.ok())
+ pc_->PostSetSessionDescriptionSuccess(wrapper_);
+ else
+ pc_->PostSetSessionDescriptionFailure(wrapper_, error.message());
+ }
+
+ private:
+ rtc::scoped_refptr<PeerConnection> pc_;
+ rtc::scoped_refptr<SetSessionDescriptionObserver> wrapper_;
+};
+
bool PeerConnectionInterface::RTCConfiguration::operator==(
const PeerConnectionInterface::RTCConfiguration& o) const {
// This static_assert prevents us from accidentally breaking operator==.
@@ -1463,7 +1492,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 (!SetLocalDescription(std::move(desc_temp), &error)) {
+ if (!SetCurrentOrPendingLocalDescription(std::move(desc_temp), &error)) {
PostSetSessionDescriptionFailure(observer, error);
return;
}
@@ -1516,9 +1545,7 @@
}
}
- SetSessionDescriptionMsg* msg = new SetSessionDescriptionMsg(observer);
- signaling_thread()->Post(RTC_FROM_HERE, this,
- MSG_SET_SESSIONDESCRIPTION_SUCCESS, msg);
+ PostSetSessionDescriptionSuccess(observer);
// According to JSEP, after setLocalDescription, changing the candidate pool
// size is not allowed, and changing the set of ICE servers will not result
@@ -1543,24 +1570,32 @@
void PeerConnection::SetRemoteDescription(
SetSessionDescriptionObserver* observer,
SessionDescriptionInterface* desc) {
+ SetRemoteDescription(
+ std::unique_ptr<SessionDescriptionInterface>(desc),
+ rtc::scoped_refptr<SetRemoteDescriptionObserverInterface>(
+ new SetRemoteDescriptionObserverAdapter(this, observer)));
+}
+
+void PeerConnection::SetRemoteDescription(
+ std::unique_ptr<SessionDescriptionInterface> desc,
+ rtc::scoped_refptr<SetRemoteDescriptionObserverInterface> 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<SessionDescriptionInterface> 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 +1603,11 @@
// 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 +1697,7 @@
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
@@ -2315,6 +2349,13 @@
observer_->OnRenegotiationNeeded();
}
+void PeerConnection::PostSetSessionDescriptionSuccess(
+ SetSessionDescriptionObserver* observer) {
+ SetSessionDescriptionMsg* msg = new SetSessionDescriptionMsg(observer);
+ signaling_thread()->Post(RTC_FROM_HERE, this,
+ MSG_SET_SESSIONDESCRIPTION_SUCCESS, msg);
+}
+
void PeerConnection::PostSetSessionDescriptionFailure(
SetSessionDescriptionObserver* observer,
const std::string& error) {
@@ -3260,7 +3301,7 @@
role);
}
-bool PeerConnection::SetLocalDescription(
+bool PeerConnection::SetCurrentOrPendingLocalDescription(
std::unique_ptr<SessionDescriptionInterface> desc,
std::string* err_desc) {
RTC_DCHECK(signaling_thread()->IsCurrent());
@@ -3316,7 +3357,7 @@
return true;
}
-bool PeerConnection::SetRemoteDescription(
+bool PeerConnection::SetCurrentOrPendingRemoteDescription(
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 2b36a86..2bbce48 100644
--- a/pc/peerconnection.h
+++ b/pc/peerconnection.h
@@ -152,6 +152,10 @@
SessionDescriptionInterface* desc) override;
void SetRemoteDescription(SetSessionDescriptionObserver* observer,
SessionDescriptionInterface* desc) override;
+ void SetRemoteDescription(
+ std::unique_ptr<SessionDescriptionInterface> desc,
+ rtc::scoped_refptr<SetRemoteDescriptionObserverInterface> observer)
+ override;
PeerConnectionInterface::RTCConfiguration GetConfiguration() override;
bool SetConfiguration(
const PeerConnectionInterface::RTCConfiguration& configuration,
@@ -271,6 +275,9 @@
~PeerConnection() override;
private:
+ class SetRemoteDescriptionObserverAdapter;
+ friend class SetRemoteDescriptionObserverAdapter;
+
struct RtpSenderInfo {
RtpSenderInfo() : first_ssrc(0) {}
RtpSenderInfo(const std::string& stream_label,
@@ -342,6 +349,8 @@
void OnVideoTrackRemoved(VideoTrackInterface* track,
MediaStreamInterface* stream);
+ void PostSetSessionDescriptionSuccess(
+ SetSessionDescriptionObserver* observer);
void PostSetSessionDescriptionFailure(SetSessionDescriptionObserver* observer,
const std::string& error);
void PostCreateSessionDescriptionFailure(
@@ -539,10 +548,15 @@
// Get current SSL role used by SCTP's underlying transport.
bool GetSctpSslRole(rtc::SSLRole* role);
- bool SetLocalDescription(std::unique_ptr<SessionDescriptionInterface> desc,
- std::string* err_desc);
- bool SetRemoteDescription(std::unique_ptr<SessionDescriptionInterface> 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<SessionDescriptionInterface> desc,
+ std::string* err_desc);
+ bool SetCurrentOrPendingRemoteDescription(
+ 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 860c6dc..0421124 100644
--- a/pc/peerconnection_rtp_unittest.cc
+++ b/pc/peerconnection_rtp_unittest.cc
@@ -33,6 +33,25 @@
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()
@@ -60,25 +79,31 @@
rtc::scoped_refptr<webrtc::PeerConnectionFactoryInterface> 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<webrtc::AudioTrackInterface> 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<webrtc::RTCError*>(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 @@
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<webrtc::RTCError*>(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<webrtc::AudioTrackInterface> 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<webrtc::RTCError*>(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<webrtc::RTCError*>(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 @@
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<webrtc::RTCError*>(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<webrtc::RTCError*>(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 @@
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()));
+ ASSERT_TRUE(
+ callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
+ static_cast<webrtc::RTCError*>(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<webrtc::RTCError*>(nullptr)));
+ ASSERT_EQ(callee->observer()->add_track_events_.size(), 2u);
EXPECT_EQ(
std::vector<rtc::scoped_refptr<webrtc::RtpReceiverInterface>>{
callee->observer()->add_track_events_[0].receiver},
@@ -158,10 +200,242 @@
// 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<webrtc::RTCError*>(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<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);
+}
+
+// Tests for the legacy SetRemoteDescription() function signature.
+class PeerConnectionRtpLegacyObserverTest : public PeerConnectionRtpTest {};
+
+// Sanity test making sure the callback is invoked.
+TEST_F(PeerConnectionRtpLegacyObserverTest, OnSuccess) {
+ auto caller = CreatePeerConnection();
+ auto callee = CreatePeerConnection();
+
+ std::string error;
+ ASSERT_TRUE(
+ callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), &error));
+}
+
+// Verifies legacy behavior: The observer is not called if if the peer
+// connection is destroyed because the asynchronous callback is executed in the
+// peer connection's message handler.
+TEST_F(PeerConnectionRtpLegacyObserverTest,
+ ObserverNotCalledIfPeerConnectionDereferenced) {
+ auto caller = CreatePeerConnection();
+ auto callee = CreatePeerConnection();
+
+ rtc::scoped_refptr<webrtc::MockSetSessionDescriptionObserver> observer =
+ new rtc::RefCountedObject<webrtc::MockSetSessionDescriptionObserver>();
+
+ auto offer = caller->CreateOfferAndSetAsLocal();
+ callee->pc()->SetRemoteDescription(observer, offer.release());
+ callee = nullptr;
+ rtc::Thread::Current()->ProcessMessages(0);
+ EXPECT_FALSE(observer->called());
+}
+
} // namespace
diff --git a/pc/peerconnectionwrapper.cc b/pc/peerconnectionwrapper.cc
index 14308b5..1b92efa 100644
--- a/pc/peerconnectionwrapper.cc
+++ b/pc/peerconnectionwrapper.cc
@@ -153,6 +153,19 @@
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 ca805a3..8918a71 100644
--- a/pc/peerconnectionwrapper.h
+++ b/pc/peerconnectionwrapper.h
@@ -90,6 +90,8 @@
// 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/test/mockpeerconnectionobservers.h b/pc/test/mockpeerconnectionobservers.h
index 9128b8c..b45c67d 100644
--- a/pc/test/mockpeerconnectionobservers.h
+++ b/pc/test/mockpeerconnectionobservers.h
@@ -213,12 +213,12 @@
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 @@
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)
