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

AudioBridge-List_Announcements #3391

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

keremcadirci
Copy link
Contributor

Requirement:

As of today, we need the file_id of a playback audio file (announcement) to stop playing.
A random admin may not know the file_id to stop it

Proposol

Add a list of announcements into listparticipants api
in that case the response of the listparticipants api would be:

{
	"audiobridge" : "participants",
	"room" : <unique numeric ID of the room>,
	"participants" : [		// Array of participant objects
		{	// Participant #1
			"id" : <unique numeric ID of the participant>,
			// ....
		},
		// Other participants
	],
	"announcements" : [		// Array of announcement objects
		{	// Announcement #1
			"file_id" : "<unique string ID of the announcement>",
			"filename": "<path to the Opus file to play>",
			"playing" : <true|false, whether or not the file is playing>,
			"loop": <true|false, depending on whether or not the file is playing in a loop forever>
		}
		// Other announcements
	]
}

@lminiero
Copy link
Member

While this is an interesting requirement, I wouldn't add this to the list of participants. It's true that we highjacked the participants structure for announcements, but they are not really participants. I'd much rather see a new "listannouncements" request or something like this, pretty much as we do for forwarders. This would allow us to differentiate announcements, and also protect them separately: to do a listforwarders, for instance, you need to know the room secret if configured, while this is not true for participants, and I think announcements are something only admins with access to the secret should have access to.

@keremcadirci
Copy link
Contributor Author

@lminiero listannouncements api added

  • Request:
{
	"request" : "listannouncements",
	"room" : <unique numeric ID of the room>
}
  • Response:
{
	"audiobridge" : "announcements",
	"room" : <unique numeric ID of the room>,
	"announcements" : [		// Array of announcement objects
		{	// Announcement #1
			"file_id" : "<unique string ID of the announcement>",
			"filename": "<path to the Opus file to play>",
			"playing" : <true|false, whether or not the file is playing>,
			"loop": <true|false, depending on whether or not the file is playing in a loop forever>
		}
		// Other announcements
	]
}

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 some notes inline.

* to be recreated (which might be needed if the audio for generated by
* the participant becomes garbled); \c rtp_forward allows you to forward
* the participants of a specific room and their details; \c listannouncements
* list all playing announcements of a specific room and their details;
Copy link
Member

Choose a reason for hiding this comment

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

lists (missing s)

\verbatim
{
"request" : "listannouncements",
"room" : <unique numeric ID of the room>
Copy link
Member

Choose a reason for hiding this comment

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

As discussed in my previous comments, I think this should be protected by the room's secret. Only admins should be allowed to see which announcements exist in the room, since a secret is required to play them in the first place.

goto prepare_response;
}
janus_refcount_increase(&audiobridge->ref);
/* Return a list of all announcements */
Copy link
Member

Choose a reason for hiding this comment

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

This is missing the secret check, as discussed in my previous comment. You can refer to how other requests handle it to see how it can be done in a short snippet. Besides, you're also missing a check on whether the audiobridge instance has already been marked as destroyed, which must happen before increasing the reference.

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.

None yet

2 participants