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

Add kick_all requests and possibility to remove PIN code to both Audiobridge and Streaming plugins #2978

Conversation

mikaelnousiainen
Copy link
Contributor

@mikaelnousiainen mikaelnousiainen commented May 11, 2022

This PR adds the following features to both the Audiobridge and the Streaming plugins:

  • A new kick_all request, which kicks and disconnects all participants in an Audiobridge room and disconnects all viewers/listeners in a Streaming plugin endpoint
  • Option to remove an existing PIN code (by setting it to an empty string) from either an Audiobridge room or a Streaming plugin endpoint

Both of these features are very useful if one needs to be able to control the set of participants/viewers in these plugins for long-lived mountpoints and/or rooms.

My specific application is using these features for providing exclusive (reserved) access to audio streams by following this process:

  1. Set a new PIN code to a Streaming plugin endpoint (or an Audiobridge room)
  2. Distribute the new PIN code to the participants/viewers allowed to access these streams
  3. Use the new kick_all requests to guarantee that there are no participants/viewers left after changing the PIN code
  4. Allow the participants with the new PIN code to join/view
  5. Once the exclusive access period is over, the PIN code is removed (set to empty string)

Copy link
Member

@lminiero lminiero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I think this may be useful in VideoRoom and TextRoom as well, so in case I'll work on a similar enhancement for those too should this be merged.

That said, I think this patch needs work. First of all, I don't think this new functionality is needed at all in the Streaming plugin, since we have a request called "disable" that is very similar, and so maybe just needs to be extended a bit if it doesn't do exactly what you need. For the AudioBridge, as I pointed out in the review below, I'd definitely cut down the potentially huge number of notifications you're generating.

I'll do a deeper code review after the patch is updated.

src/plugins/janus_audiobridge.c Outdated Show resolved Hide resolved
src/plugins/janus_streaming.c Outdated Show resolved Hide resolved
Copy link
Member

@lminiero lminiero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Added a few notes inline.

src/plugins/janus_audiobridge.c Show resolved Hide resolved
src/plugins/janus_streaming.c Outdated Show resolved Hide resolved
src/plugins/janus_streaming.c Show resolved Hide resolved
src/plugins/janus_streaming.c Outdated Show resolved Hide resolved
src/plugins/janus_streaming.c Show resolved Hide resolved
src/plugins/janus_streaming.c Outdated Show resolved Hide resolved
src/plugins/janus_streaming.c Outdated Show resolved Hide resolved
@mikaelnousiainen
Copy link
Contributor Author

mikaelnousiainen commented May 19, 2022

@lminiero Changes done, please have a look at the refcounts and mutexes, as I'm not 100% sure about the ordering.

Also, in Streaming plugin destroy event handling, there is mutex locking on session->mutex too, like this: https://github.com/meetecho/janus-gateway/pull/2978/files#diff-3d84c5c61736914d38e005f37baaecfa324295ff90a45962773a0cb90520f2beL4482

Is this needed with kick_all? I'm simply locking each individual session (s->mutex) for every viewer, as they are modified.

@lminiero
Copy link
Member

Just as a quick note, I haven't forgotten about this! I'll have to review the use of mutexes as you suggested, and I'll do it in the next few days.

Copy link
Member

@lminiero lminiero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes! They all look fine to me. The note you see below is a memo to me, so something I have to fix in the existing code myself.

I'll make a couple of tests later, and in case I don't see issues, I'll merge 👍

src/plugins/janus_streaming.c Show resolved Hide resolved
@lminiero lminiero added the multistream Related to Janus 1.x label May 30, 2022
@lminiero
Copy link
Member

lminiero commented Jun 8, 2022

Thanks for the quick fix! Finally merging then 💪

@lminiero lminiero merged commit e5b25bb into meetecho:master Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multistream Related to Janus 1.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants