Skip to content

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
Contributor

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
Contributor

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
Contributor

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