Add text message support for data channel send and receive callbacks.

[torepaulsson]

Finally got around to this update. I've updated the solution to not be a breaking change. Added new method for sending text messages over the data channel. Added a new event which is fired when non-binary data is received.

This will still be a change in the inner workings of the mr-webrtc dll. I've opened up an issue to discuss what can be done about it below!

[ghost]

All CLA requirements met.

[djee-ms]

Hi Tore, thanks for your contribution. Unfortunately I don't think we'll be able to take that change as is:

• I have yet to understand what's the story with this binary flag. First, very practically, nobody ever complained about it, everyone as I know is passing data around as binary even if the content is some text. I don't immediately see what would be the benefit of telling the protocol that your data is a string, as opposed to leaving it as an opaque binary blob (which can perfectly hold a string as far as I can tell; I know some users are passing JSON data without any issue). Second, more theoretically, I have looked at the RFC and as far as I can tell the protocol allows you to tell it that you send text, but doesn't seem to otherwise care about it (except for the fact this guarantees UTF-8), and therefore I don't see why one would bother informing the protocol about this if it doesn't change anything. Really the only case I see for this (and incidentally this is the same story with the WebSocket protocol) is to send and receive data between peers that have zero priori knowledge nor implicit agreement on the content, and use the protocol to enforce UTF-8 encoding. But as soon as you assume any encoding, then my understanding is that binary is completely equivalent.

• This is a breaking change that essentially breaks the entire data channel API, and so far I have not seen compelling reason to do so. And you mentioned yourself a non-API-breaking alternative.

If you can explain why this is so critical and why sending text in a binary blob is impractical for your scenario, then we can consider a change. Your issue #673 states that there's no other way, but as explained I am fairly confident encoding the string as UTF-8 (or anything else, really) yourself and sending it as binary should work, unless you have a specific remote endpoint implementation with other restrictions (and if so, which one?). And if we take a change anyway, which I am open to do if that can help unblock you, I would really want to avoid an API breaking and instead would strongly prefer having an incremental change with a new overload specifically for text data, as you suggested.

[torepaulsson]

Hi Jerome, thanks for your quick reply! I agree that this cannot be included if the feature create a breaking change! I will update the draft with non-breaking changes. I will keep the nuget package as is and only add overloads+one event for the text data. I will also add methods and events in the mr-webrtc so that also does not get any breaking changes.

This feature is really not a real use case for this library I guess. The issue I have is that I use it for communicating with another library, webrtc in gstreamer, in the implementation I have the commands are sent using the text messages and the data is sent with the binary flag.

I agree that practically it makes little sense to have this flag, bytes are bytes, and as long as the two parties that communicate agree on a protocol everything is fine, but I cannot change the other party in this instance :)

[djee-ms]

Hi @taurepaulsson,

Sorry for the delay. We have been talking internally with @fibann about this change, as I had mixed feelings about it and how it's currently implemented. And I think we agree that we'd want instead to leave the current behavior as is, and add an overload (say, SendMessageEx / MessageReceivedEx) that has the binary/text flag exposed. That way users could still use the current API as is, or could use this new API to send/receive a mix of text and binary messages, and know about it. There are 2 related reasons for wanting this:

• Your current implementation actually does change the behavior, because the MessageReceived callback is made to only be called for binary message, whereas it was previously called for all messages. To that end, this is a breaking change. For an existing user upgrading to a version with your change, they would suddenly lose some messages unless they also subscribe to the new TextMessageReceived callback.

• More generally, adding a functionality via an overload / separate method without touching any of the existing API feels like a safer approach to ensure this is an incremental feature and not a breaking change.

Would you mind giving it a try with that approach? I looked at your implementation otherwise and there was nothing else really to change in terms of code quality or missing comment or anything I think, just this change in behavior of MessageReceived that we'd like to avoid.

Thanks!

[torepaulsson]

Hi @djee-ms No worries, we all have other things to do as well, I'm very glad that you help me out with this. I agree about your proposal regarding the MessageReceivedEx event.

Regarding the change in the implementation I've tried to add it that the event is always fired, unless you register to the text message callback. So it should not be a breaking change now But I will change the event to function as you say instead, create a extended version and always fire both events if registered, a lot easier to understand!

Regarding the SendMessage I feel that it is nice that it takes a string instead of a byte array? What do you think? It wont be breaking if I just add a SendMessage(string message) overload to the existing SendMessage method. If you feel strong about the SendMessageEx I can create that as well! Just wanted to ask.

Thanks for the help!

[djee-ms]

I've tried to add it that the event is always fired, unless you register to the text message callback. So it should not be a breaking change

You're right, I missed that sorry. I still prefer the separate overload which avoids a behind-the-scene change of behavior of the event depending on some other state, I think it's easier to understand for users.

Regarding the SendMessage I feel that it is nice that it takes a string instead of a byte array?

Sorry this was a bit confusing. I was talking about the C interop API, where you cannot/shouldn't use overloads, e.g. mrsDataChannelSendMessageEx(). Similarly we want I think an event mrsDataChannelMessageExCallback similar to the existing one but with an extra parameter for the text/binary kind. With those two, we can implement all the overloads we want in any language.

Now, for the C# library there are already several Send overloads:

• SendMessage(byte[] message) is the historical one, most convenient for binary data.
• SendMessage(IntPtr message, ulong size) is an optimized overload that prevent allocation, because the byte[] array above was forcing developers to allocate an array in some circumstances.

My proposal is to add:

• SendMessageEx(MessageKind messageKind, IntPtr message, ulong size) which would be the "raw" overload directly mapping to mrsDataChannelSendMessageEx().
• SendMessage(string textMessage) which I agree with you is nice to have, but should clearly document it sends a text message only, not binary.

For the receive event, we should also add an Ex handler I guess. I don't see a better way:

  public event Action<byte[]> MessageReceived;
+ public event Action<MessageKind, byte[]> MessageReceivedEx;

Side note too, as shown above I'd avoid bool and use an enum to be 100% explicit about the message kind, something like:

enum MessageKind
{
    Binary,
    Text
}

How does that sound @torepaulsson? Also ping @fibann if he wants to chime in.

[torepaulsson]

Hi @djee-ms Thanks :) I'll give it a go!

[torepaulsson]

Hi @djee-ms, Lot of meetings today but I've changed the implementation to what you suggested. I wondered a bit if you wanted an enum also in the C++ mrwebrtc project, I included one but maybe it would be easier to implement interops by using the boolean flag.

Hopefully I've gotten it the way you wanted :)

[djee-ms]

All good for me. @torepaulsson if you want to add your name to the CONTRIBUTORS list you can update this PR with it. Otherwise I'll merge it as is.

[torepaulsson]

All good for me. @torepaulsson if you want to add your name to the CONTRIBUTORS list you can update this PR with it. Otherwise I'll merge it as is.

Hi, I've rebased so the fixup commit is included in the first commit. It is OK, no need to put me as a contributor :)

[djee-ms]

Merged, thanks for your contribution @torepaulsson !!

[Qualcuno]

Good job. Sending text is very important if a peer is on another system such as an SFU which distinguishes between text messages and binary messages. When will this feature be available in the nuget package? https://www.nuget.org/packages/Microsoft.MixedReality.WebRTC.UWP

[torepaulsson]

@Qualcuno, I don't know, I'm curious as well though! @djee-ms, do you know the plans for a future release?