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

Allow one session to join multiple rooms #55

Open
devfans opened this issue Apr 25, 2020 · 12 comments
Open

Allow one session to join multiple rooms #55

devfans opened this issue Apr 25, 2020 · 12 comments

Comments

@devfans
Copy link

devfans commented Apr 25, 2020

Hi, I was checking the plugin implementation, now thinking it looks possible to allow one session to join multiple rooms. Currently, I only need the publisher to be across rooms though. Wondering are you planning to implement this feature? Or is it impossible to add this feature for some logic I didn't notice?

Thanks!

@mrhegemon
Copy link

We are building a naf-jitsi-adapter with sfu support. This should work with hubs but will need many reticulum services. https://myxr.social

@devfans
Copy link
Author

devfans commented Apr 27, 2020

Hi @gfodor, wondering which commit of this plugin is the hubs live env building with? Because I was building from the master branch of this plugin today and put the .so file on the Janus server and restarted the process, then the process will throw a segment error when processing JSEP. Also, I see the branch feature/user-room-m2m, so does it already support one Janus session to join multiple rooms? Thanks

@gfodor
Copy link
Contributor

gfodor commented Apr 27, 2020 via email

@devfans
Copy link
Author

devfans commented Apr 28, 2020

I see, thanks. The issue was probably caused by plugin API compatibility since I saw some recent updates that were added for supporting the new Janus version.

@mqp
Copy link
Contributor

mqp commented Apr 28, 2020 via email

@devfans
Copy link
Author

devfans commented Apr 28, 2020

Hi, I was trying with Janus 0.7.x I think, I saw this commit #55 was added to support newer Janus version, so that's probably why I was running into an issue.

@mqp
Copy link
Contributor

mqp commented Apr 28, 2020

Yes, unfortunately there's no way to keep the plugin backwards-compatible with old Janus versions. Commit 14a3346 is the last one that should be compatible with 0.7.x.

@vincentfretin
Copy link
Contributor

With the current code in master, for a publisher to be in several rooms at the same time, the following needs to be modified:

  • process_join needs to be modified to have a list of rooms in parameters. This is a breaking change so better doing a process_join_multi_rooms with new MessageKind::MultiJoin something like that, and you call switchboard.join_publisher for each room (and notify the other publishers in the room) and set from.join_state with room_ids plural instead of a single room_id.
  • process_block/process_unblock needs changes to iterate over publisher rooms to find the session (with notifications:true) of participant we want to block. Actually we can just use get_publisher there, we have an optimized struct now. In process_join we should maybe check notifications: true instead of data: true (the hack) to know if it's a publisher?
  • In destroy_session, you need to iterate over the rooms the publisher are in to notify the other participants he left.

I was just checking what needs to be modified in the code to support this use case now that I'm familiar with the code, but I don't have this use case for now. I know @arpu you were interested having such a feature.

@devfans
Copy link
Author

devfans commented Feb 12, 2021

The use case was a publisher’s voice needed to broadcast to lots of small rooms. I made a patch to support that https://github.com/devfans/janus-plugin-sfu/tree/mcc

@vincentfretin
Copy link
Contributor

vincentfretin commented Feb 12, 2021

Ah ah nice @devfans you almost exactly did what I said. :D It's just a little bit hacky with room_id.split("-") to have several rooms instead of my MultiJoin message I proposed, but it works I guess. :)
Here the link with all the changes to better see what changed with your 4 commits
devfans/janus-plugin-sfu@master...devfans:mcc

I see you did a change in process_data (broadcast to all rooms) and data_recipients_for (using only the first/main room). Can you explain the use case you had here so I better understand the changes? Because for me the changes in process_data may not be what I expect if I use this for chat. I want conversation to be isolated for a room, eventually a moderator can broadcast to all rooms why not if we want to do some sort of breakout rooms, a use case I'm really interested in. Maybe we need to add a room_ids parameter for the MessageKind::Data, if not specified broadcast to all rooms, otherwise broadcast to the specified rooms.

To move this further, I think we can start doing a PR just for process_block/process_unblock to use get_publisher like I said, this should be a small change without breaking anything. => #80
After that a second PR with changes similar to what you did and like I proposed. If other can explain their use cases it would be great so we can take decisions how to implement this so it covers all your use cases.

@devfans
Copy link
Author

devfans commented Feb 12, 2021

oh right, it was used for Hubs conference, which required the speakers’s voice to be broadcast to the guest rooms. So the solution was either the speakers to joins all the rooms and publish streams without subscribing to the guest rooms, or all guests need to joins the speakers room to subscribe to the speakers but without publishing. So the main room you mentioned is the original primary room in the context.

@vincentfretin
Copy link
Contributor

Ok I understand your use case, in the end this was mainly to have more users listening to the speaker :-)
If you were using Hubs Cloud, I understand better why you did the changes like this, this way you didn't have to change the js code I guess, no changes in naf-janus-adapter was required. Note that the changes you did in process_data weren't required for your case I think because Hubs doesn't use the Data message via janus, but uses the Phoenix channel. process_data is called only when you use NAF.connection.adapter.reliableTransport = "websocket"; and/or NAF.connection.adapter.unreliableTransport = "websocket"; Hubs is using a function for those that send data to the Phoenix socket.

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

No branches or pull requests

5 participants