Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement WebRTC Data Channel. #1238

Merged
merged 15 commits into from
Jul 7, 2021

Conversation

ctrlaltdavid
Copy link
Collaborator

@ctrlaltdavid ctrlaltdavid commented May 31, 2021

Implements the WebRTC data channel.

This PR is targeted at the webapp feature branch, where Web app-related changes will be accumulated and tested before being periodically merged into master (and vice-versa).

It builds upon PR #1221 which needs to be merged first. The changes in this PR start in commit 6c37628.

QA: See the Vircadia-Web-SDK PR: vircadia/vircadia-web-sdk#6 See the Vircadia-Web-SDK PR: vircadia/vircadia-web-sdk#10

Merge sequence:

@ctrlaltdavid ctrlaltdavid added do not merge do not merge due to issues or pending updates needs testing (QA) The PR is ready for testing needs CR (code review) labels May 31, 2021
@ctrlaltdavid ctrlaltdavid removed the do not merge do not merge due to issues or pending updates label Jun 27, 2021
@digisomni digisomni added QA Approved The PR has been tested successfully. and removed needs testing (QA) The PR is ready for testing labels Jun 27, 2021
libraries/networking/CMakeLists.txt Outdated Show resolved Hide resolved
libraries/networking/src/webrtc/WebRTCDataChannels.cpp Outdated Show resolved Hide resolved
#ifdef WEBRTC_DEBUG
qCDebug(networking_webrtc) << "WDCConnection::sendDataMessage()";
#endif
const int MAX_WEBRTC_BUFFER_SIZE = 16 * 1024 * 1024; // 16MB
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Constant probably should be somewhere else, perhaps at the top of the file?
Why 16M? Is this a protocol issue, or just a sane memory limit?

What kind of data will be sent? Textures may exceed 16M quite easily.

There should be some sort of log message here, too.

Copy link
Collaborator Author

@ctrlaltdavid ctrlaltdavid Jul 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a WebRTC protocol limit in the WebRTC Send() method. This is just how much data can be buffered to send. Ultimately, it's called via Socket::writeDatagram() - in PR1259 - and UDP sockets can similarly fail to write date. The Vircadia protocol handles this situation (dropping unreliable packets and resending reliable packets). And large data items are chopped up into multi-packet messages with the packets sent individually by this method (or similar UDP method).


#if defined(Q_OS_MAC)
# define WEBRTC_AUDIO 1
# define WEBRTC_POSIX 1
# define WEBRTC_LEGACY 1
#elif defined(Q_OS_WIN)
# define WEBRTC_AUDIO 1
# define WEBRTC_DATA_CHANNEL 1
# define WEBRTC_DATA_CHANNELS 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the constants should be named something like ENABLE_WEBRTC_DATA_CHANNELS instead, for clarity?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the "topic" of the define ("WEBRTC") coming first makes it easier to distinguish these defines from other defines and group related ones together.

@ctrlaltdavid ctrlaltdavid added needs testing (QA) The PR is ready for testing and removed QA Approved The PR has been tested successfully. labels Jul 4, 2021
@ctrlaltdavid
Copy link
Collaborator Author

ctrlaltdavid commented Jul 5, 2021

Latest changes should be QA'd using Web SDK PR vircadia/vircadia-web-sdk#10

@ctrlaltdavid ctrlaltdavid moved this from In progress to Review in progress in Web Part 1: Interface + SDK - May through October 2021 Jul 5, 2021
@ctrlaltdavid ctrlaltdavid added enhancement New feature or request web sdk Related to the Vircadia Web SDK labels Jul 6, 2021
@digisomni digisomni added QA Approved The PR has been tested successfully. and removed needs testing (QA) The PR is ready for testing labels Jul 6, 2021
@digisomni
Copy link
Member

Le Wild Kalila has tested this again with vircadia/vircadia-web-sdk PR #10 by running a basic test suite.

Web Part 1: Interface + SDK - May through October 2021 automation moved this from Review in progress to Reviewer approved Jul 7, 2021
@digisomni digisomni added CR Approved At least one code reviewer has approved the PR. and removed needs CR (code review) labels Jul 7, 2021
@digisomni digisomni merged commit 5a2ca48 into vircadia:webapp Jul 7, 2021
Web Part 1: Interface + SDK - May through October 2021 automation moved this from Reviewer approved to Done Jul 7, 2021
@ctrlaltdavid ctrlaltdavid deleted the dev/webrtc-datachannel branch July 8, 2021 03:53
@digisomni digisomni changed the title WebRTC Data Channel Implement WebRTC Data Channel. Sep 7, 2021
@digisomni digisomni added unmerged-dev A project being developed in parallel to master, to be merged at a later date. and removed enhancement New feature or request labels Sep 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CR Approved At least one code reviewer has approved the PR. QA Approved The PR has been tested successfully. unmerged-dev A project being developed in parallel to master, to be merged at a later date. web sdk Related to the Vircadia Web SDK
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants