meetecho / janode

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

audiobridge join does not seem to return AUDIOBRIDGE_EVENT_JOINED #8

Closed augustblack closed 2 years ago

augustblack commented 2 years ago

when I do audioHandle.join(...)

I get Promise<{ feed, room, participants }> in return.

This matches janus except that it seems to return without the 'display' field.

However, the docs seem to indicate it should return Promise

Maybe just a documentation error?

atoppi commented 2 years ago

That is a documentation error. AUDIOBRIDGE_EVENT_JOINED is not correctly defined.

atoppi commented 2 years ago

I've double-checked how those events are emitted from Janus, fixed the payloads and the documentation accordingly. The commit is https://github.com/meetecho/janode/commit/d82f0c068ded9830188ebe7e2a3fef8fe0f36b59.

Please verify that everything works as expected.

augustblack commented 2 years ago

looks good!

The 'display' is still not being returned on the join request.

Is that maybe a janus issue?

atoppi commented 2 years ago

The Janus doc mentions the display in the joined event, but the actual code does not insert that field in the json. I could add the display by returning the one passed by the user.

augustblack commented 2 years ago

I think ideally, janus should really include the newly joined user in the participants array. But, it doesn't.

Maybe it is better to try and fix it in janus? I would suggest that Janus return participants with the newly joined user. If that can't be done, then returning display should suffice.

atoppi commented 2 years ago

Janus notifies the other participants in the room about the new peer in a joined event that contains a participants array with just one element (that is the new peer). In that element Janus includes also the display.

When it comes to the participant that just joined, that joined event will include also the participants array, but this array will omit the newly joined peer. Are you suggesting to add the peer itself in the participants array of the joined event that the newly joined peer will receive? If that is the case, it would be a breaking change and I guess Lorenzo will not accept it.

augustblack commented 2 years ago

yes, that is what I am suggesting. In my experience, these kinds of exchanges should be chunky and not chatty. The way janus handles it (generally speaking), it forces the client to piece together the list of participants that janus already has as a whole. It is imho very error prone. I would prefer that instead of all of the 'peer-leaving' and 'peer-joining' type events, that janus would send a simple 'participants-update' event with the new list of participants that includes everyone. A breaking change would be bad news, all around. So, I wouldn't suggest it if that is really the case.

I guess the cool thing about janode is that we can now more easily shape those events ourselves on the server side.

It should suffice to maybe ask Lorenzo to return the "display" like the janus docs say should happen.

thanks for your attention on this and for the janode library!

atoppi commented 2 years ago

Closing the issue since the original problem has been solved. Please open a new issue for other questions.