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

Publisher in multiple rooms #91

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

vincentfretin
Copy link
Contributor

@vincentfretin vincentfretin commented Jul 10, 2021

I rewrote @devfans changes against master #55 (comment)
This includes the commit from #90. I will rebase once #90 is merged.
This may not be the final implementation, maybe adding an additional message or modifying an existing message may be better.
I post it as draft if anyone want to play with it.

To play with it, in the naf-janus-adapter repo

cp examples/index.html examples/1.html # leave the room as is: room:1
cp examples/index.html examples/2.html # edit 2.html to set room:2
cp examples/index.html examples/12.html # edit 12.html to set room:1-2

open in 3 different tabs:

In the browser console type the following:

First user is in room 1 and room 2:

> NAF.clientId
"3079162068668260"
> NAF.connection.adapter.availableOccupants
[]
> NAF.connection.adapter.mediaStreams
{3079162068668260: {…}}

Second user in room 1:

> NAF.clientId
"2058753888545049"
> NAF.connection.adapter.availableOccupants
["3079162068668260"]
> NAF.connection.adapter.mediaStreams
{2058753888545049: {…}, 3079162068668260: {…}}

Third user in room 2:

> NAF.clientId
"2094703467261257"
> NAF.connection.adapter.availableOccupants
["3079162068668260"]
> NAF.connection.adapter.mediaStreams
{2094703467261257: {…}, 3079162068668260: {…}}

So all availableOccupants and mediaStreams are what we expect. Avatars and properly hearing the audio of those avatars is another thing.

If second user (room 1 tab) connected after the first user (publisher in tab 12.html), issue this command for the first user

document.getElementById('player').components.networked.syncAll(null, true)

to force sending the entities (the avatar) to participants of main room (room 1), this will create the publisher avatar (and so the networked-audio-source) in room 1 tab.

Why the publisher avatar appears in room 1 tab when it connects after the others?
Because the entities are sent with this.syncAll(undefined, true); on each networked component via the connected listener when the user connect.
The other case where the entities are sent is when a datachannel open when we subscribe to a user, it calls this.entities.completeSync(clientId, true) that calls syncAll on each entity, this is not the case here because the publisher subscribe to no one, this is why the publisher avatar doesn't appear if a participant goes into room 1 after the publisher.

For room 2:
We never see the first user (publisher) avatar (so no audio) because the naf updates doesn't go to room 2 (because we use datachannel transport and we have &joined.room_ids[0] in data_recipients_for used by incoming_data). The first user (publisher) mediastream is there though, you can listen to it:

let publisherId = Object.keys(NAF.connection.adapter.mediaStreams).filter(key => key != NAF.clientId)[0]; // take the first mediastream that is not me, this works only with two users in the room
const audio = document.createElement('audio');
audio.srcObject = NAF.connection.adapter.mediaStreams[publisherId].audio;
audio.play();

In 12.html tab, there are some strange things happening but explainable:

Now if we use websocket transport instead of datachannel.
In the html files, add in adapter-ready listener:

adapter.unreliableTransport = "websocket";
adapter.reliableTransport = "websocket";

instead of incoming_data, this will use process_data that broadcasts naf updates to all rooms the publisher is in, so creating the publisher avatar in room 2 tab as well (the issue of syncAll explained above still applies).
But we still have the issue of ghost avatars in publisher room (12.html). This needs changes in naf-janus-adapter where we have the various this.room checks.
The publisher doesn't hear others, that's expected for performance but you may want to at least hear the users in the main room (room 1).

So yeah here is the state of things. You will need changes in naf-janus-adapter and in your app to make use of this for a given use case.
The function incoming_data should probably do the same thing that process_data to broadcasts to all rooms I think, it doesn't make sense that it's different, well it may depend of the use case.
I don't have a clear use case myself for now. :)

@vincentfretin
Copy link
Contributor Author

See networked-aframe/naf-janus-adapter#15 if you want to play with the example I described.

@vincentfretin
Copy link
Contributor Author

The main issue I see is that the publisher (speaker) is receiving naf updates (u, um, r messages) from all avatars from all rooms, this may be a big perf issue for the publisher if they publishes in 10 rooms with 20 avatars each. You can ignore on the client side the messages by setting NAF.options.syncSource="ignore-all" (this is really a hack so it triggers this code path) but you're still receiving naf updates you don't need. The publisher won't see any avatars and so won't hear any participants in this case.

So the changes here may be interesting only with an alternative transport other than "datachannel" (which uses incoming_data) or "websocket" (which uses process_data) transports for the naf updates, like Phoenix channel that Hubs is using (here in hubs and here in reticulum) so the naf messages only broadcast to the room channel you're in (room "1" for example), but you can still broadcast the audio in other rooms by setting networked-scene room (room "1-2-3-4-5" to broadcast to rooms 1,2,3,4,5 for example)

@vincentfretin
Copy link
Contributor Author

While I'm thinking about it, I'm adding a note here. If you have some horizontal scaling implemented in your infra to scale the rooms on several janus instances, you need to make sure all the rooms 1,2,3,4,5 goes into the same janus instance.

@vincentfretin
Copy link
Contributor Author

vincentfretin commented Jul 12, 2021

After some thinking, some changes I may do to this PR:

  • in process_join, check JWT for all rooms, not just the main room
  • in process_join, return the users of each room, so the speaker know how many users there is in each room and can maintain the list of users of each room via the join and leave events (we have the room_id in those events)
  • check if room is full for secondary rooms? I'll say no, it would be sad that the speaker can't broadcast to a room because it's full
  • add a room_ids parameter for the MessageKind::Data, if not specified broadcast to all rooms, otherwise broadcast to the specified rooms. Be careful on security, we still need to verify the user is really in those rooms.
    (test some chat feature through the websocket transport, be sure that the speaker can choose to send a text message only to a single room or to all rooms)
  • When using datachannel transport for naf updates, change data_recipients_for behavior used by incoming_data to broadcast to all rooms instead of main room? In the end no I don't think it makes sense for some naf updates that shouldn't be broadcasted to several rooms. We can't easily do a filtering on room_id like we could for process_data.
  • in naf-janus-adapter, add room_ids: [main_room] in all naf messages when I'm in several rooms so we can only broadcast to a single room and not all rooms (This will be only used with the "websocket" transport.)
  • To fix the issue that the publisher is receiving naf updates from all the rooms, in process_data, broadcast only if the occupant main room match the room_id given in parameter? Only for naf messages? So if \"dataType\":\"u\" or {\"dataType\":\"um\" or {\"dataType\":\"r\" found in body string?
  • instead of doing the hack parse_rooms and split on "-", introduce new messages to start and stop broadcasting to secondary rooms and send new events broadcastingStart/broadcastingStop (similiar to join/leave) so we can create some speaker avatar with a networked-audio-source.

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.

1 participant