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

Add WebRTC Data Channel Support to streaming-client.js #38

Merged
merged 6 commits into from
Mar 2, 2024

Conversation

guilherme-marcello
Copy link
Contributor

Description

This pull request adds support for WebRTC data channels in the streaming-client.js file. It enables the creation of a data channel named "channel" in the PeerConnection and includes the necessary event handling for the data channel's onopen and onmessage events. Additionally, binary data received through the data channel is properly decoded using a TextDecoder instance.

Changes Made

  • Created a new TextDecoder instance for decoding data.
  • Added code to create a WebRTC data channel in the PeerConnection.
  • Handled the ondatachannel event to set up callbacks for onopen and onmessage.
  • Implemented decoding of binary data using the TextDecoder.
  • Logged the decoded data to the console for easy observation.

Testing

Tested the changes by integrating them with a Janus instance and verified that the data channel works as expected. Verified proper logging of both regular and binary data in the browser console.

Copy link
Member

@atoppi atoppi left a comment

Choose a reason for hiding this comment

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

thanks for the effort, left a bunch of comments

@@ -10,6 +10,8 @@ const remoteVideo = document.getElementById('remoteVideo');
const myStream = parseInt(getURLParameter('stream')) || 1;
const myPin = getURLParameter('pin') || null;

const decoder = new TextDecoder();
Copy link
Member

Choose a reason for hiding this comment

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

datachannels are optional, do not initialize the decoder here, just declare a variable

let decoder;

@@ -282,6 +284,22 @@ async function doAnswer(offer) {
//'sdpSemantics': 'unified-plan',
});

// DataChannel
pc.createDataChannel("channel");
Copy link
Member

Choose a reason for hiding this comment

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

  1. use name JanusDataChannel, since that is the one that Janus will use
  2. again, datachannels are optional so inspect the offer.sdp for m=application lines before creating the DC, e.g.
if (/m=application [1-9]\d*/.test(offer.sdp)) {
  // create dc and set callbacks
}

// DataChannel
pc.createDataChannel("channel");
pc.ondatachannel = (event) => {
const channel = event.channel;
Copy link
Member

Choose a reason for hiding this comment

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

please add a log here to notify a remote datachannel, e.g.

console.log(`new remote datachannel: ${channel.id} (${channel.label})`);

if (event.data?.byteLength) { // is ArrayBuffer
decodedData = decoder.decode(event.data);
}
console.log(decodedData);
Copy link
Member

Choose a reason for hiding this comment

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

specify in the log message that the origin is datachannel e.g.

console.log(`datchannel ${channel.id} (${channel.label}) received`, decodedData);

channel.onmessage = (event) => {
let decodedData = event.data;
if (event.data?.byteLength) { // is ArrayBuffer
decodedData = decoder.decode(event.data);
Copy link
Member

Choose a reason for hiding this comment

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

here you can initialize the decoder

decoder = decoder || new TextDecoder()

pc.ondatachannel = (event) => {
const channel = event.channel;
channel.onopen = (event) => {
console.log("[pc.ondatachannel] Opening data channel!");
Copy link
Member

Choose a reason for hiding this comment

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

change log message in

console.log(`datchannel ${channel.id} (${channel.label}) open`);

@atoppi
Copy link
Member

atoppi commented Feb 29, 2024

@guilherme-marcello are you still interested in merging this?

@guilherme-marcello
Copy link
Contributor Author

Hello @atoppi !

Thank you for pointing out areas for improvement with such detail, I totally agree with your suggestions!

I've added the suggested changes into the code, and I believe the enhancements will positively impact the functionality and maintainability of the WebRTC data channel handling in streaming-client.js, as I needed myself during a project.

Thank you once again for your review!

@atoppi
Copy link
Member

atoppi commented Mar 1, 2024

Sorry if maybe I was not clear enough.

Check how janus.js handles dcs:

https://github.com/meetecho/janus-gateway/blob/master/html/demos/janus.js#L1650

Basically you need to do both things if data are going to be negotiated:

  • create a local dc with label JanusDataChannel
  • listen for pc.ondatachannel remote dc

For both channels, set up onopen/message/error/close callbacks with proper logging (local/remote and label)

@guilherme-marcello
Copy link
Contributor Author

Hello!

Thank you for the explanation. Hope is working properly right now.

Cheers!

@atoppi
Copy link
Member

atoppi commented Mar 2, 2024

Thanks, looking good now.
Have you tested it?

If it's working I'll merge and then fix some small issues (basically linting and variable/function naming) by myself.

@guilherme-marcello
Copy link
Contributor Author

Yes, it is working; I tested before pushing. Same tests as before, with the multistream version of Janus, checking both regular and binary data in the browser console.

@atoppi atoppi merged commit 084aadc into meetecho:master Mar 2, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants