Skip to content

feat: Add data_type configuration to PeerConnection.send_data #200

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

Merged

Conversation

SamWolfs
Copy link
Contributor

  • Added a parameter to the PeerConnection.send_data function to allow for the choice between :binary and :string.
  • Added a simple test to verify the new PeerConnection.send_data/4 arity.

No tests were added to validate the correctness of the payload, as it's handled by ex_sctp and would require some sort of passthrough mock. Please let me know if this would require testing and what the preferred approach would be.

@mickel8
Copy link
Member

mickel8 commented Feb 24, 2025

Hi @SamWolfs!

Thanks for the PR. I wonder what's the use-case for choosing between string and binary? Checkin the official JS API, I can't see this info being exposed in any-way?

@SamWolfs
Copy link
Contributor Author

SamWolfs commented Feb 24, 2025

Hi @mickel8

We're sending audio with an unsupported media type over the DataChannel to a GStreamer DataChannel Peer, which cares about the payload type.

Edit: As you may find in the diff, there was a TODO mentioning that this should be implemented in the future.

@mickel8
Copy link
Member

mickel8 commented Feb 24, 2025

Yeah, I just wonder what's the difference between string and binary as this option seems not to be exposed in JS API, yet you can still send files (hence binaries). Maybe it always should be a binary 🤔

@SamWolfs
Copy link
Contributor Author

SamWolfs commented Feb 24, 2025

Other libraries/frameworks definitely seem to care about the payload type https://github.com/pion/webrtc/blob/44062a7a78006d7e6b2f97d991ad3c12a648150c/datachannelmessage.go#L9 which is also being set based on this parameter in elixir-webrtc/ex_sctp.

@mickel8
Copy link
Member

mickel8 commented Feb 24, 2025

Yeah, I also took a look here: https://www.rfc-editor.org/rfc/rfc8831.html#section-5-6

So PPI (Payload Protocol Identifier) is passed by upper layer and then provided to the upper layer on the other side. It's not meant for SCTP itself. What's more in JS API it's not available (I assume they always use binary).

However, if other "backend" WebRTCimplementations care about it, I think we can make it configurable.

Thanks for the PR!

@mickel8 mickel8 merged commit 241931e into elixir-webrtc:master Feb 24, 2025
@mickel8 mickel8 mentioned this pull request Mar 6, 2025
63 tasks
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