Merge pull request #255 from paullouisageneau/fix-renegociation-role

Fix roles during renegociation

include/rtc/peerconnection.hpp

@@ -129,7 +129,7 @@ public:
 	void onTrack(std::function<void(std::shared_ptr<Track> track)> callback);
 
 private:
-	std::shared_ptr<IceTransport> initIceTransport(Description::Role role);
+	std::shared_ptr<IceTransport> initIceTransport();
 	std::shared_ptr<DtlsTransport> initDtlsTransport();
 	std::shared_ptr<SctpTransport> initSctpTransport();
 	void closeTransports();

src/icetransport.cpp

@@ -46,11 +46,12 @@ using std::chrono::system_clock;
 
 namespace rtc {
 
-IceTransport::IceTransport(const Configuration &config, Description::Role role,
-                           candidate_callback candidateCallback, state_callback stateChangeCallback,
+IceTransport::IceTransport(const Configuration &config, candidate_callback candidateCallback,
+                           state_callback stateChangeCallback,
                            gathering_state_callback gatheringStateChangeCallback)
-    : Transport(nullptr, std::move(stateChangeCallback)), mRole(role), mMid("0"),
-      mGatheringState(GatheringState::New), mCandidateCallback(std::move(candidateCallback)),
+    : Transport(nullptr, std::move(stateChangeCallback)), mRole(Description::Role::ActPass),
+      mMid("0"), mGatheringState(GatheringState::New),
+      mCandidateCallback(std::move(candidateCallback)),
       mGatheringStateChangeCallback(std::move(gatheringStateChangeCallback)),
       mAgent(nullptr, nullptr) {
 
@@ -139,13 +140,19 @@ Description IceTransport::getLocalDescription(Description::Type type) const {
 	if (juice_get_local_description(mAgent.get(), sdp, JUICE_MAX_SDP_STRING_LEN) < 0)
 		throw std::runtime_error("Failed to generate local SDP");
 
+	// RFC 5763: The endpoint that is the offerer MUST use the setup attribute value of
+	// setup:actpass.
+	// See https://tools.ietf.org/html/rfc5763#section-5
 	return Description(string(sdp), type,
 	                   type == Description::Type::Offer ? Description::Role::ActPass : mRole);
 }
 
 void IceTransport::setRemoteDescription(const Description &description) {
-	mRole = description.role() == Description::Role::Active ? Description::Role::Passive
-	                                                        : Description::Role::Active;
+	if (mRole == Description::Role::ActPass)
+		mRole = description.role() == Description::Role::Active ? Description::Role::Passive
+		                                                        : Description::Role::Active;
+	if (mRole == description.role())
+		throw std::logic_error("Incompatible roles with remote description");
 
 	mMid = description.bundleMid();
 	if (juice_set_remote_description(mAgent.get(),
@@ -316,11 +323,12 @@ void IceTransport::LogCallback(juice_log_level_t level, const char *message) {
 
 namespace rtc {
 
-IceTransport::IceTransport(const Configuration &config, Description::Role role,
-                           candidate_callback candidateCallback, state_callback stateChangeCallback,
+IceTransport::IceTransport(const Configuration &config, candidate_callback candidateCallback,
+                           state_callback stateChangeCallback,
                            gathering_state_callback gatheringStateChangeCallback)
-    : Transport(nullptr, std::move(stateChangeCallback)), mRole(role), mMid("0"),
-      mGatheringState(GatheringState::New), mCandidateCallback(std::move(candidateCallback)),
+    : Transport(nullptr, std::move(stateChangeCallback)), mRole(Description::Role::ActPass),
+      mMid("0"), mGatheringState(GatheringState::New),
+      mCandidateCallback(std::move(candidateCallback)),
       mGatheringStateChangeCallback(std::move(gatheringStateChangeCallback)),
       mNiceAgent(nullptr, nullptr), mMainLoop(nullptr, nullptr) {
 
@@ -526,12 +534,21 @@ Description IceTransport::getLocalDescription(Description::Type type) const {
 
 	std::unique_ptr<gchar[], void (*)(void *)> sdp(nice_agent_generate_local_sdp(mNiceAgent.get()),
 	                                               g_free);
-	return Description(string(sdp.get()), type, mRole);
+
+	// RFC 5763: The endpoint that is the offerer MUST use the setup attribute value of
+	// setup:actpass.
+	// See https://tools.ietf.org/html/rfc5763#section-5
+	return Description(string(sdp.get()), type,
+	                   type == Description::Type::Offer ? Description::Role::ActPass : mRole);
 }
 
 void IceTransport::setRemoteDescription(const Description &description) {
-	mRole = description.role() == Description::Role::Active ? Description::Role::Passive
-	                                                        : Description::Role::Active;
+	if (mRole == Description::Role::ActPass)
+		mRole = description.role() == Description::Role::Active ? Description::Role::Passive
+		                                                        : Description::Role::Active;
+	if (mRole == description.role())
+		throw std::logic_error("Incompatible roles with remote description");
+
 	mMid = description.bundleMid();
 	mTrickleTimeout = !description.ended() ? 30s : 0s;
 

src/icetransport.hpp

@@ -45,8 +45,8 @@ public:
 	using candidate_callback = std::function<void(const Candidate &candidate)>;
 	using gathering_state_callback = std::function<void(GatheringState state)>;
 
-	IceTransport(const Configuration &config, Description::Role role,
-	             candidate_callback candidateCallback, state_callback stateChangeCallback,
+	IceTransport(const Configuration &config, candidate_callback candidateCallback,
+	             state_callback stateChangeCallback,
 	             gathering_state_callback gatheringStateChangeCallback);
 	~IceTransport();
 

src/peerconnection.cpp

@@ -187,13 +187,7 @@ void PeerConnection::setLocalDescription(Description::Type type) {
 	}
 	}
 
-	auto iceTransport = std::atomic_load(&mIceTransport);
-	if (!iceTransport) {
-		// RFC 5763: The endpoint that is the offerer MUST use the setup attribute value of
-		// setup:actpass.
-		// See https://tools.ietf.org/html/rfc5763#section-5
-		iceTransport = initIceTransport(Description::Role::ActPass);
-	}
+	auto iceTransport = initIceTransport();
 
 	Description localDescription = iceTransport->getLocalDescription(type);
 	processLocalDescription(std::move(localDescription));
@@ -273,9 +267,7 @@ void PeerConnection::setRemoteDescription(Description description) {
 	auto remoteCandidates = description.extractCandidates();
 	auto type = description.type();
 
-	auto iceTransport = std::atomic_load(&mIceTransport);
-	if (!iceTransport)
-		iceTransport = initIceTransport(Description::Role::ActPass);
+	auto iceTransport = initIceTransport();
 
 	iceTransport->setRemoteDescription(description);
 	processRemoteDescription(std::move(description));
@@ -405,14 +397,14 @@ void PeerConnection::onTrack(std::function<void(std::shared_ptr<Track>)> callbac
 	mTrackCallback = callback;
 }
 
-shared_ptr<IceTransport> PeerConnection::initIceTransport(Description::Role role) {
+shared_ptr<IceTransport> PeerConnection::initIceTransport() {
 	PLOG_VERBOSE << "Starting ICE transport";
 	try {
 		if (auto transport = std::atomic_load(&mIceTransport))
 			return transport;
 
 		auto transport = std::make_shared<IceTransport>(
-		    mConfig, role, weak_bind(&PeerConnection::processLocalCandidate, this, _1),
+		    mConfig, weak_bind(&PeerConnection::processLocalCandidate, this, _1),
 		    [this, weak_this = weak_from_this()](IceTransport::State state) {
 			    auto shared_this = weak_this.lock();
 			    if (!shared_this)