Merge pull request #390 from paullouisageneau/fix-dc-double-offer

Fix DataChannel stream number assignment on double offer

src/impl/datachannel.cpp

@@ -170,6 +170,7 @@ size_t DataChannel::maxMessageSize() const {
 }
 
 void DataChannel::shiftStream() {
+	std::shared_lock lock(mMutex);
 	if (mStream % 2 == 1)
 		mStream -= 1;
 }

src/impl/peerconnection.cpp

@@ -262,6 +262,9 @@ shared_ptr<SctpTransport> PeerConnection::initSctpTransport() {
 		if (!remote || !remote->application())
 			throw std::logic_error("Starting SCTP transport without application description");
 
+		// This is the last occasion to ensure the stream numbers are coherent with the role
+		shiftDataChannels();
+
 		uint16_t sctpPort = remote->application()->sctpPort().value_or(DEFAULT_SCTP_PORT);
 		auto lower = std::atomic_load(&mDtlsTransport);
 		auto transport = std::make_shared<SctpTransport>(
@@ -549,8 +552,7 @@ void PeerConnection::forwardBufferedAmount(uint16_t stream, size_t amount) {
 		channel->triggerBufferedAmount(amount);
 }
 
-shared_ptr<DataChannel> PeerConnection::emplaceDataChannel(Description::Role role, string label,
-                                                           DataChannelInit init) {
+shared_ptr<DataChannel> PeerConnection::emplaceDataChannel(string label, DataChannelInit init) {
 	std::unique_lock lock(mDataChannelsMutex); // we are going to emplace
 	uint16_t stream;
 	if (init.id) {
@@ -558,6 +560,13 @@ shared_ptr<DataChannel> PeerConnection::emplaceDataChannel(Description::Role rol
 		if (stream == 65535)
 			throw std::invalid_argument("Invalid DataChannel id");
 	} else {
+		// RFC 5763: The answerer MUST use either a setup attribute value of setup:active or
+		// setup:passive. [...] Thus, setup:active is RECOMMENDED.
+		// See https://tools.ietf.org/html/rfc5763#section-5
+		// Therefore, we assume passive role if we are the offerer.
+		auto iceTransport = getIceTransport();
+		auto role = iceTransport ? iceTransport->role() : Description::Role::Passive;
+
 		// RFC 8832: The peer that initiates opening a data channel selects a stream identifier for
 		// which the corresponding incoming and outgoing streams are unused.  If the side is acting
 		// as the DTLS client, it MUST choose an even stream identifier; if the side is acting as
@@ -907,6 +916,10 @@ void PeerConnection::processRemoteDescription(Description description) {
 	auto iceTransport = initIceTransport();
 	iceTransport->setRemoteDescription(std::move(description));
 
+	// Since we assumed passive role during DataChannel creation, we might need to shift the stream
+	// numbers from odd to even.
+	shiftDataChannels();
+
 	if (description.hasApplication()) {
 		auto dtlsTransport = std::atomic_load(&mDtlsTransport);
 		auto sctpTransport = std::atomic_load(&mSctpTransport);

src/impl/peerconnection.hpp

@@ -65,8 +65,7 @@ struct PeerConnection : std::enable_shared_from_this<PeerConnection> {
 	void forwardBufferedAmount(uint16_t stream, size_t amount);
 	optional<string> getMidFromSsrc(uint32_t ssrc);
 
-	shared_ptr<DataChannel> emplaceDataChannel(Description::Role role, string label,
-	                                           DataChannelInit init);
+	shared_ptr<DataChannel> emplaceDataChannel(string label, DataChannelInit init);
 	shared_ptr<DataChannel> findDataChannel(uint16_t stream);
 	void shiftDataChannels();
 	void iterateDataChannels(std::function<void(shared_ptr<DataChannel> channel)> func);

src/peerconnection.cpp

@@ -223,11 +223,6 @@ void PeerConnection::setRemoteDescription(Description description) {
 		// This is an offer, we need to answer
 		if (!impl()->config.disableAutoNegotiation)
 			setLocalDescription(Description::Type::Answer);
-	} else {
-		// This is an answer
-		// Since we assumed passive role during DataChannel creation, we need to shift the
-		// stream numbers by one to shift them from odd to even.
-		impl()->shiftDataChannels();
 	}
 
 	for (const auto &candidate : remoteCandidates)
@@ -250,14 +245,7 @@ optional<string> PeerConnection::remoteAddress() const {
 }
 
 shared_ptr<DataChannel> PeerConnection::createDataChannel(string label, DataChannelInit init) {
-	// RFC 5763: The answerer MUST use either a setup attribute value of setup:active or
-	// setup:passive. [...] Thus, setup:active is RECOMMENDED.
-	// See https://tools.ietf.org/html/rfc5763#section-5
-	// Therefore, we assume passive role when we are the offerer.
-	auto iceTransport = impl()->getIceTransport();
-	auto role = iceTransport ? iceTransport->role() : Description::Role::Passive;
-
-	auto channelImpl = impl()->emplaceDataChannel(role, std::move(label), std::move(init));
+	auto channelImpl = impl()->emplaceDataChannel(std::move(label), std::move(init));
 	auto channel = std::make_shared<DataChannel>(channelImpl);
 
 	if (auto transport = impl()->getSctpTransport())