[jitsi-dev] [jitsi-videobridge] Add a generic 'endpoint' colibri message that allows clients to commu… (#91)


#1

…nicate an arbitrary payload to one another/an entire conference.

I thought I'd run this idea by the jitsi team. I wanted to add a generic colibri message that applications can use to communicate application-specific information over the data channel. Our use cases centered around endpoints being able to send information in either '1:1' (endpoint sends a message specifically to another endpoint) or 'broadcast' (endpoint sends a message to everyone else in the conference) modes. This way applications can send their own messages over the existing datachannel when connecting via the bridge.

The way I've got it formatted now is this:
message {
  colibriClass: "EndpointMessage",
  msgPayload: <arbitrary message payload, jitsi doesn't care, up to application developer>
}
but I'm open to suggestions, this was just what I threw together to match our use case. Let me know if this is a feature that you think would make sense to be included upstream.
You can view, comment on, or merge this pull request online at:

  https://github.com/jitsi/jitsi-videobridge/pull/91

-- Commit Summary --

  * Add a generic 'endpoint' colibri message that allows clients to communicate an arbitrary payload to one another/an entire conference.

-- File Changes --

    M src/main/java/org/jitsi/videobridge/Conference.java (22)
    M src/main/java/org/jitsi/videobridge/Endpoint.java (38)

-- Patch Links --

https://github.com/jitsi/jitsi-videobridge/pull/91.patch
https://github.com/jitsi/jitsi-videobridge/pull/91.diff

···

to: "broadcast" | <user_id [the id used by the bridge]>,

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/91


#2

Sending messages between endpoints over the data channel can have many different use-cases, so I think defining a generic mechanism as you have done is a very good idea. I think we should include it.

In the format for a broadcast, I would rather use 'to: ""', or use a separate colibriClass (e.g. BroadcastMessage), so that "broadcast" is free for use as an endpoint ID.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/91#issuecomment-146993183


#3

the right thing to do (from a protocol perspective) would not be to put everything on the same datachannel and reinvent the whole (de)multiplexing done by sctp and webrtc datachannels.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/91#issuecomment-146998533


#4

thanks guys.

@fippo sounds interesting...can you elaborate on what you mean?

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/91#issuecomment-147008595


#5

@brianh5 the webrtc peerconnection supports a (nearly) arbitrary number of different datachannels running over it. Using different datachannels would prevent issues like filetransfers blocking active speaker notifications.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/91#issuecomment-147116918


#6

Ah, I understand now. The intent for this message wrapper was for messages that are very similar to the events Jitsi already passes over this datachannel (normal signaling-type messages, not things like file transfer), but that are application specific; so to me it felt like re-using the existing connection made sense. I do totally agree for things of a different nature (like file transfer) would be best handled with a separate one.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/91#issuecomment-147446536


#7

the thing about being application specific is that you need to demultiplex them somehow. Which would be best done by the datachannel's protocol property. That way, you've separated your application concerns from the jvb's concerns.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/91#issuecomment-147452280


#8

The way I saw it, demuxing these messages would be no different than the ones already sent by the bridge. Each type (dominantlastspeaker, lastn, etc.) needs to be tested for and handled with a specific method, I saw the things going into the application message being handled the same way. Or, detected as an application method, and a handler is invoked that the application can register for to handle. To me it didn't feel like that would be muddying the waters too much. I had viewed this more as a generic wrapper to support messages over an existing channel.

Admittedly, the idea of spinning up a new data channel didn't occur to me. I don't know if there are any other issues there (e.g. any overhead, any testing/verification the bridge may need with regards to setting up multiple data channels, etc.). It seems like the jitsi code (referring to the jitsi javascript library, which the application uses) would have to at least give the hooks for creating this second data channel and allow the application to register a handler for it.

Not sure how things like this are typically settled, I'm happy to defer to the Jitsi team if they think this isn't what's desired upstream.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/91#issuecomment-148210910


#9

Also I agree with Boris that we should either use an empty endpoint "" instead of "broadcast".

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/91#issuecomment-151637404


#10

@bgrozev @paweldomas
So I've just made a few tweaks:
1) Cleaned up the system.out.println logging
2) Used empty string to denote 'broadcast' rather than a hard-coded string

I also made an attempt at documenting the behavior and some of the caveats we talked about (in onEndpointMessage)--let me know if you guys think that strikes the right tone.

If it looks good, let me know and I can squash the commits before you merge it in.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/91#issuecomment-152265157


#11

@brianh5 LGTM
Two minor things: by our code convention, all lines should be wrapped at 80 chars. To avoid code duplication, the existing broadcastMessageOnDataChannels should be changed to use sendMessageOnDataChannels.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/91#issuecomment-152279911


#12

@bgrozev thanks boris, made those tweaks.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/91#issuecomment-153133639


#13

Merged #91.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/91#event-453458361


#14

I'd like to comment on this too, since it's (fairly recent):

The way I saw it, demuxing these messages would be no different than the ones already sent by the bridge. Each type (dominantlastspeaker, lastn, etc.) needs to be tested for and handled with a specific method, I saw the things going into the application message being handled the same way. Or, detected as an application method, and a handler is invoked that the application can register for to handle. To me it didn't feel like that would be muddying the waters too much. I had viewed this more as a generic wrapper to support messages over an existing channel.

I agree with Fippo. The argument you're making here is that because the bridge is already using a single channel for multiple things, it justifies continuing to do so. In line with Fippo's suggestions, the `dominantEndpoint` and `lastN` messages should be on separate channels too.

I find it quite confusing that I'm getting arbitrary messages, `dominantEndpoint` messages, and `lastN` messages, all over a single channel called, `lastN` - they should each have a discrete channel.

That said, I do appreciate you doing this, as I was able to make use of it today.

Additionally, I'd like to see more `docs` writeups for features like this. I can help contribute to those missing as well, but new features should always come with documentation.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/91#issuecomment-162993420


#15

I'm not sure a unique data channel per message type feels like the way to go. To me it seems like having them at a higher level based on the context and category of traffic makes more sense. I don't think we need unique channels for `dominantEndpoint` and `lastN`--not only do these messages have a heavy contextual overlap, but their transmission characteristics (frequency, size, etc.) are similar. Now, should we introduce something like file transfer, which has not only a different context but very different transmission characteristics that could cause issues for other messages, that seems like something deserving of its own channel.

I didn't actually know the current channel was named `lastN`, but it just seems like a naming problem to me.

Just my thoughts. I definitely agree it would be nice to have support for easily spinning up new data channels as needed.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/91#issuecomment-163067853