From 6b188d888ec5df42a13c6c527fdf845c02e23678 Mon Sep 17 00:00:00 2001 From: David Rowe Date: Mon, 28 Jun 2021 11:15:18 +1200 Subject: [PATCH 1/5] Update to latest Vircadia-Web --- vircadia-web | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vircadia-web b/vircadia-web index ac1f13a39c..ab6c8b1a54 160000 --- a/vircadia-web +++ b/vircadia-web @@ -1 +1 @@ -Subproject commit ac1f13a39c702ee54bf2cda8bc35e5d34f7f0756 +Subproject commit ab6c8b1a54aec359b1894f70722f69cfec7f04f1 From dacda8405c170d1ecd8ea333267deabd1f2e1d8a Mon Sep 17 00:00:00 2001 From: David Rowe Date: Sat, 3 Jul 2021 08:43:31 +1200 Subject: [PATCH 2/5] Typo --- libraries/networking/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/networking/CMakeLists.txt b/libraries/networking/CMakeLists.txt index 1835c7a6cd..7a11329a14 100644 --- a/libraries/networking/CMakeLists.txt +++ b/libraries/networking/CMakeLists.txt @@ -11,7 +11,7 @@ endif () if (WIN32) # we need ws2_32.lib on windows, but it's static so we don't bubble it up - # Libraries needed for WebRTC: security.log winmm.lib + # Libraries needed for WebRTC: security.lib winmm.lib target_link_libraries(${TARGET_NAME} ws2_32.lib security.lib winmm.lib) elseif(APPLE) # IOKit is needed for getting machine fingerprint From f6a8ae285db16d48de9cb35e23c49820bf1b5fb3 Mon Sep 17 00:00:00 2001 From: David Rowe Date: Sun, 4 Jul 2021 15:50:52 +1200 Subject: [PATCH 3/5] Fix WebRTC peer connection not being closed properly --- .../src/webrtc/WebRTCDataChannels.cpp | 94 +++++++++++++++---- .../src/webrtc/WebRTCDataChannels.h | 31 +++++- 2 files changed, 100 insertions(+), 25 deletions(-) diff --git a/libraries/networking/src/webrtc/WebRTCDataChannels.cpp b/libraries/networking/src/webrtc/WebRTCDataChannels.cpp index 497ecf9a55..48ec1b7b9b 100644 --- a/libraries/networking/src/webrtc/WebRTCDataChannels.cpp +++ b/libraries/networking/src/webrtc/WebRTCDataChannels.cpp @@ -22,7 +22,7 @@ const std::string ICE_SERVER_URI = "stun://ice.vircadia.com:7337"; -#define WEBRTC_DEBUG +// #define WEBRTC_DEBUG void WDCSetSessionDescriptionObserver::OnSuccess() { @@ -102,6 +102,10 @@ void WDCPeerConnectionObserver::OnDataChannel(rtc::scoped_refptronPeerConnectionStateChanged(newState); } @@ -253,6 +257,20 @@ void WDCConnection::sendIceCandidate(const IceCandidateInterface* candidate) { _parent->sendSignalingMessage(jsonObject); } +void WDCConnection::onPeerConnectionStateChanged(PeerConnectionInterface::PeerConnectionState state) { +#ifdef WEBRTC_DEBUG + const char* STATES[] = { + "New", + "Connecting", + "Connected", + "Disconnected", + "Failed", + "Closed" + }; + qCDebug(networking_webrtc) << "WDCConnection::onPeerConnectionStateChanged() :" << (int)state << STATES[(int)state]; +#endif +} + void WDCConnection::onDataChannelOpened(rtc::scoped_refptr dataChannel) { #ifdef WEBRTC_DEBUG qCDebug(networking_webrtc) << "WDCConnection::onDataChannelOpened() :" @@ -276,16 +294,19 @@ void WDCConnection::onDataChannelOpened(rtc::scoped_refptr void WDCConnection::onDataChannelStateChanged() { auto state = _dataChannel->state(); #ifdef WEBRTC_DEBUG - qCDebug(networking_webrtc) << "WDCConnection::dataChannelStateChanged() :" << (int)state + qCDebug(networking_webrtc) << "WDCConnection::onDataChannelStateChanged() :" << (int)state << DataChannelInterface::DataStateString(state); #endif if (state == DataChannelInterface::kClosed) { - _dataChannel->Close(); + // Close data channel. + _dataChannel->UnregisterObserver(); + _dataChannelObserver = nullptr; _dataChannel = nullptr; - // WEBRTC FIXME: The following line causes the _peerConnectionFactory to fail. - //_peerConnection->Close(); - //_peerConnection = nullptr; - _parent->onDataChannelClosed(this, _dataChannelID); +#ifdef WEBRTC_DEBUG + qCDebug(networking_webrtc) << "Disposed of data channel"; +#endif + // Close peer connection. + _parent->closePeerConnection(this); } } @@ -320,6 +341,18 @@ bool WDCConnection::sendDataMessage(const DataBuffer& buffer) { return _dataChannel->Send(buffer); } +void WDCConnection::closePeerConnection() { +#ifdef WEBRTC_DEBUG + qCDebug(networking_webrtc) << "WDCConnection::closePeerConnection()"; +#endif + _peerConnection->Close(); + _peerConnection = nullptr; + _peerConnectionObserver = nullptr; +#ifdef WEBRTC_DEBUG + qCDebug(networking_webrtc) << "Disposed of peer connection"; +#endif +} + WebRTCDataChannels::WebRTCDataChannels(NodeType_t nodeType, QObject* parent) : _nodeType(nodeType), @@ -348,6 +381,9 @@ WebRTCDataChannels::WebRTCDataChannels(NodeType_t nodeType, QObject* parent) : if (!_peerConnectionFactory) { qCWarning(networking_webrtc) << "Failed to create WebRTC peer connection factory"; } + + // Set up mechanism for closing peer connections. + connect(this, &WebRTCDataChannels::closePeerConnectionSoon, this, &WebRTCDataChannels::closePeerConnectionNow); } WebRTCDataChannels::~WebRTCDataChannels() { @@ -384,18 +420,6 @@ void WebRTCDataChannels::onDataChannelOpened(WDCConnection* connection, quint16 _connectionsByDataChannel.insert(dataChannelID, connection); } -void WebRTCDataChannels::onDataChannelClosed(WDCConnection* connection, quint16 dataChannelID) { -#ifdef WEBRTC_DEBUG - qCDebug(networking_webrtc) << "WebRTCDataChannels::onDataChannelClosed() :" << dataChannelID; -#endif - - // Delete WDCConnection. - _connectionsByWebSocket.remove(connection->getWebSocketID()); - _connectionsByDataChannel.remove(dataChannelID); - // WEBRTC FIXME: The following line causes the _peerConnectionFactory to fail. - //delete connection; -} - void WebRTCDataChannels::onSignalingMessage(const QJsonObject& message) { #ifdef WEBRTC_DEBUG qCDebug(networking_webrtc) << "WebRTCDataChannel::onSignalingMessage()" << message; @@ -481,12 +505,42 @@ rtc::scoped_refptr WebRTCDataChannels::createPeerConnec configuration.servers.push_back(iceServer); #ifdef WEBRTC_DEBUG - qCDebug(networking_webrtc) << "2. Create a new PeerConnection"; + qCDebug(networking_webrtc) << "2. Create a new peer connection"; #endif PeerConnectionDependencies dependencies(peerConnectionObserver.get()); auto result = _peerConnectionFactory->CreatePeerConnection(configuration, std::move(dependencies)); +#ifdef WEBRTC_DEBUG + qCDebug(networking_webrtc) << "Created peer connection"; +#endif return result; } +void WebRTCDataChannels::closePeerConnection(WDCConnection* connection) { +#ifdef WEBRTC_DEBUG + qCDebug(networking_webrtc) << "WebRTCDataChannels::closePeerConnection()"; +#endif + // Use Qt's signals/slots mechanism to close the peer connection on its own call stack, separate from the DataChannel + // callback that initiated the peer connection. + // https://bugs.chromium.org/p/webrtc/issues/detail?id=3721 + emit closePeerConnectionSoon(connection); +} + + +void WebRTCDataChannels::closePeerConnectionNow(WDCConnection* connection) { +#ifdef WEBRTC_DEBUG + qCDebug(networking_webrtc) << "WebRTCDataChannels::closePeerConnectionNow()"; +#endif + // Close the peer connection. + connection->closePeerConnection(); + + // Delete the WDCConnection. + _connectionsByWebSocket.remove(connection->getWebSocketID()); + _connectionsByDataChannel.remove(connection->getDataChannelID()); + delete connection; +#ifdef WEBRTC_DEBUG + qCDebug(networking_webrtc) << "Disposed of connection"; +#endif +} + #endif // WEBRTC_DATA_CHANNELS diff --git a/libraries/networking/src/webrtc/WebRTCDataChannels.h b/libraries/networking/src/webrtc/WebRTCDataChannels.h index d9bb213a1d..0b8caf80b5 100644 --- a/libraries/networking/src/webrtc/WebRTCDataChannels.h +++ b/libraries/networking/src/webrtc/WebRTCDataChannels.h @@ -48,6 +48,7 @@ public: /// @brief A WebRTC create session description observer. class WDCCreateSessionDescriptionObserver : public CreateSessionDescriptionObserver { public: + WDCCreateSessionDescriptionObserver(WDCConnection* parent); /// @brief The call to CreateAnswer succeeded. @@ -118,6 +119,7 @@ private: class WDCConnection { public: + /// @brief Constructs a new WDCConnection and opens a WebRTC data connection. /// @param webSocketID The signaling channel that initiated the opening of the WebRTC data channel. /// @param parent The parent WebRTCDataChannels object. @@ -155,6 +157,10 @@ public: /// @param candidate The ICE candidate. void sendIceCandidate(const IceCandidateInterface* candidate); + /// @brief Monitors the peer connection state. + /// @param state The new peer connection state. + void onPeerConnectionStateChanged(PeerConnectionInterface::PeerConnectionState state); + /// @brief Handles the WebRTC data channel being opened. /// @param dataChannel The WebRTC data channel. void onDataChannelOpened(rtc::scoped_refptr dataChannel); @@ -171,6 +177,9 @@ public: /// @param buffer The message to send. /// @return `true` if the message was sent, otherwise `false`. bool sendDataMessage(const DataBuffer& buffer); + + /// @brief Closes the WebRTC peer connection. + void closePeerConnection(); private: WebRTCDataChannels* _parent; @@ -228,11 +237,6 @@ public: /// @param dataChannelID The WebRTC data channel ID. void onDataChannelOpened(WDCConnection* connection, quint16 dataChannelID); - /// @brief Handles a WebRTC data channel closing. - /// @param connection The WebRTC data channel connection. - /// @param dataChannelID The WebRTC data channel ID. - void onDataChannelClosed(WDCConnection* connection, quint16 dataChannelID); - /// @brief Emits a signalingMessage to be sent to the Interface client. /// @param message The WebRTC signaling message to send. void sendSignalingMessage(const QJsonObject& message); @@ -254,12 +258,24 @@ public: rtc::scoped_refptr createPeerConnection( const std::shared_ptr peerConnectionObserver); + /// @brief Initiates closing the peer connection for a WebRTC data channel. + /// @details Emits a {@link WebRTCDataChannels.closePeerConnectionSoon} signal which is connected to + /// {@link WebRTCDataChannels.closePeerConnectionNow} in order to close the peer connection on a new call stack. This is + /// necessary to work around a WebRTC library limitation. + /// @param connection The WebRTC data channel connection. + void closePeerConnection(WDCConnection* connection); + public slots: /// @brief Handles a WebRTC signaling message received from the Interface client. /// @param message The WebRTC signaling message. void onSignalingMessage(const QJsonObject& message); + /// @brief Closes the peer connection for a WebRTC data channel. + /// @details Used by {@link WebRTCDataChannels.closePeerConnection}. + /// @param connection The WebRTC data channel connection. + void closePeerConnectionNow(WDCConnection* connection); + signals: /// @brief A WebRTC signaling message to be sent to the Interface client. @@ -273,6 +289,11 @@ signals: /// @param byteArray The Vircadia protocol message. void dataMessage(int dataChannelID, const QByteArray& byteArray); + /// @brief Signals that the peer connection for a WebRTC data channel should be closed. + /// @details Used by {@link WebRTCDataChannels.closePeerConnection}. + /// @param connection The WebRTC data channel connection. + void closePeerConnectionSoon(WDCConnection* connection); + private: QObject* _parent; From e682336cc205ea4e0fc9b957efd32e3c34539e83 Mon Sep 17 00:00:00 2001 From: David Rowe Date: Sun, 4 Jul 2021 15:57:53 +1200 Subject: [PATCH 4/5] Add missing Doxygen --- libraries/networking/src/webrtc/WebRTCDataChannels.h | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/libraries/networking/src/webrtc/WebRTCDataChannels.h b/libraries/networking/src/webrtc/WebRTCDataChannels.h index 0b8caf80b5..3edf9f9b9a 100644 --- a/libraries/networking/src/webrtc/WebRTCDataChannels.h +++ b/libraries/networking/src/webrtc/WebRTCDataChannels.h @@ -49,6 +49,8 @@ public: class WDCCreateSessionDescriptionObserver : public CreateSessionDescriptionObserver { public: + /// @brief Constructs a session description observer. + /// @param parent The parent connection object. WDCCreateSessionDescriptionObserver(WDCConnection* parent); /// @brief The call to CreateAnswer succeeded. @@ -67,6 +69,9 @@ private: /// @brief A WebRTC peer connection observer. class WDCPeerConnectionObserver : public PeerConnectionObserver { public: + + /// @brief Constructs a peer connection observer. + /// @param parent The parent connection object. WDCPeerConnectionObserver(WDCConnection* parent); /// @brief Called when the SignalingState changes. @@ -100,6 +105,9 @@ private: /// @brief A WebRTC data channel observer. class WDCDataChannelObserver : public DataChannelObserver { public: + + /// @brief Constructs a data channel observer. + /// @param parent The parent connection object. WDCDataChannelObserver(WDCConnection* parent); /// @brief The data channel state changed. From 7ecd9b6b8e83456715e90407667c91f35991e39e Mon Sep 17 00:00:00 2001 From: David Rowe Date: Sun, 4 Jul 2021 16:38:47 +1200 Subject: [PATCH 5/5] Miscellaneous tidying --- .../networking/src/webrtc/WebRTCDataChannels.cpp | 12 +++++++----- libraries/networking/src/webrtc/WebRTCDataChannels.h | 4 ++-- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/libraries/networking/src/webrtc/WebRTCDataChannels.cpp b/libraries/networking/src/webrtc/WebRTCDataChannels.cpp index 48ec1b7b9b..69c954ad4b 100644 --- a/libraries/networking/src/webrtc/WebRTCDataChannels.cpp +++ b/libraries/networking/src/webrtc/WebRTCDataChannels.cpp @@ -21,6 +21,7 @@ // - https://webrtc.googlesource.com/src/+/master/api/peer_connection_interface.h const std::string ICE_SERVER_URI = "stun://ice.vircadia.com:7337"; +const int MAX_WEBRTC_BUFFER_SIZE = 16777216; // 16MB // #define WEBRTC_DEBUG @@ -128,9 +129,9 @@ void WDCDataChannelObserver::OnMessage(const DataBuffer& buffer) { } -WDCConnection::WDCConnection(quint16 webSocketID, WebRTCDataChannels* parent) : - _webSocketID(webSocketID), - _parent(parent) +WDCConnection::WDCConnection(WebRTCDataChannels* parent, quint16 webSocketID) : + _parent(parent), + _webSocketID(webSocketID) { #ifdef WEBRTC_DEBUG qCDebug(networking_webrtc) << "WDCConnection::WDCConnection() :" << webSocketID; @@ -333,10 +334,11 @@ bool WDCConnection::sendDataMessage(const DataBuffer& buffer) { #ifdef WEBRTC_DEBUG qCDebug(networking_webrtc) << "WDCConnection::sendDataMessage()"; #endif - const int MAX_WEBRTC_BUFFER_SIZE = 16 * 1024 * 1024; // 16MB if (_dataChannel->buffered_amount() + buffer.size() > MAX_WEBRTC_BUFFER_SIZE) { // Don't send, otherwise the data channel will be closed. return false; + } else { + qCDebug(networking_webrtc) << "WebRTC send buffer overflow"; } return _dataChannel->Send(buffer); } @@ -440,7 +442,7 @@ void WebRTCDataChannels::onSignalingMessage(const QJsonObject& message) { if (_connectionsByWebSocket.contains(from)) { connection = _connectionsByWebSocket.value(from); } else { - connection = new WDCConnection(from, this); + connection = new WDCConnection(this, from); _connectionsByWebSocket.insert(from, connection); } diff --git a/libraries/networking/src/webrtc/WebRTCDataChannels.h b/libraries/networking/src/webrtc/WebRTCDataChannels.h index 3edf9f9b9a..75325781ed 100644 --- a/libraries/networking/src/webrtc/WebRTCDataChannels.h +++ b/libraries/networking/src/webrtc/WebRTCDataChannels.h @@ -129,9 +129,9 @@ class WDCConnection { public: /// @brief Constructs a new WDCConnection and opens a WebRTC data connection. - /// @param webSocketID The signaling channel that initiated the opening of the WebRTC data channel. /// @param parent The parent WebRTCDataChannels object. - WDCConnection(quint16 webSocketID, WebRTCDataChannels* parent); + /// @param webSocketID The signaling channel that initiated the opening of the WebRTC data channel. + WDCConnection(WebRTCDataChannels* parent, quint16 webSocketID); /// @brief Gets the WebSocket ID. /// @return The ID of the WebSocket.