search

meetecho / janode

A Node.js adapter for the Janus WebRTC server
ISC License
98 stars 36 forks source link

Make videoroom compatible with multistream #17

Closed golgetahir closed 1 year ago

golgetahir commented 2 years ago

Hi @atoppi I am using Janode in the multistream version, so far these changes need to be done for making it work on the plug-in end. I hope it will be helpful, if you decide to merge it I can contribute further by developing a sample for it etc.

golgetahir commented 1 year ago

Hi @atoppi thanks a lot for your care on the subject. Of course, we are on the same page that this is just a patch on the plug-in to somewhat support the new architecture, also I am using a custom version of the server side handler. Though I am willing to work as a contribution if you guide me a bit about how shall we complete the task. If it makes sense for you too feel free to reach me, tahir.golge@gmail.com is my e-mail address and we can work closer you can assign me tasks etc.

atoppi commented 1 year ago

Hi @golgetahir and everyone interested in this topic! In the last few days I thought about how to proceed here and came to the conclusion that maybe the best approach would be to create a whole new vr multistream plugin either by implementing a totally different unit (e.g. videoroom-multi-plugin.js) or by introducing some kind of versioning in the existing plugin (I'd prefer the former solution to be honest).

I feel that making the existing plugin compatible with multistream: 1) without breaking previous apps 2) supporting both flavors of API in the same file unit

would lead to over-complicated and hardly maintainable/readable code.

atoppi commented 1 year ago

@golgetahir I ended up importing some multistream events and properties in the videoroom plugin in order to make existing apps compatibile with janus 1.x. Would you please update the PR by solving conflicts?

Janus 1.x has some major differences when dealing with particular scenarios: e.g. subscriber renegotiations are totally different, since 0.x sends back an updated JSEP, while 1.x sends a publisher list event with an updated streams array, and it's up to the application decide to renegotiate or not through the update API.

I deliberately decided not to import the code for joining a subscriber in multistream mode (e.g. adding the streams property to join API), like this PR is doing, since that would lead the server to interact in 1.x mode and I guess that would break a whole bunch of existing applications if misused.

Unfortunately I still have to find the time to better understand the consequences of the migration. Nonetheless I feel we are doing some steps in the right direction, thanks for the support!

golgetahir commented 1 year ago

Hi @atoppi sorry that it took a while for me to take a look at, had some nasty events ( earthquake ) in Turkey but I am returning my normal routines now, I updated the PR but I think nothing left from this PR is going to be merged if you deleted the join APIs streams method ( if I understand it correctly ).

I think you are right about the breaking changes. If there is anything I can do please let me know.

atoppi commented 1 year ago

@golgetahir I saw on the news, what a catastrophic and terrible event... hope you are all doing well and safe!

I guess the PR is complete now, thanks again for the effort!

atoppi commented 1 year ago

Sorry for the huge delay, I forgot this was a pending PR. I have superseded this PR with the commit above, thanks again!

(potential problems with existing applications will be handled by applications or by opening new issues)