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-stop_all_files #3392

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

Conversation

keremcadirci
Copy link
Contributor

Requirement:

One or more announcement from file can be played by an admin in the audiobridge room
An admin without knowing file_ids may want to stop all announcements.
Currently we need to know all file_ids and we need call many stop_file api to stop all annoucements.

Proposol

A new stop_all_files api that will stop all playing announcements.
A announcement-stopped event will be fired for all stopped announcements
stop_all_files api will return stopped file_id list

  • API Request:
{
	"request" : "stop_all_files",
	"room" : <unique numeric ID of the room where the playback is taking place>,
	"secret" : "<room password, if configured>"
}
  • API Responce:
{
	"audiobridge" : "success",
	"room" : <unique numeric ID, same as request>,
	"file_id_list" : [
		// Array of file_Id: "<unique string ID of the now interrupted announcement>""
		]
}

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.

"room" : <unique numeric ID, same as request>,
"file_id_list" : [
// Array of file_Id: "<unique string ID of the now interrupted announcement>""
]
Copy link
Member

Choose a reason for hiding this comment

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

Indentation seems broken here (there's one extra tab).

goto prepare_response;
}

/* Get list of started announcements and send a stop announcement notification*/
Copy link
Member

Choose a reason for hiding this comment

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

There's a missing space at the end of the sentence (the asterisk of the comment is right next to the last word).

janus_refcount_decrease(&p->ref);
}

/* Get rid of the announcements */
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why this is a separate block. You were already looping on all announcements before, so that's where you can populate the array too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like either but since we are removing an element (g_hash_table_remove) inside audiobridge->anncs inside the loop, while loop above crashes with g_hash_table_iter_next: assertion 'ri->version' == ri->hash_table->version.... error.
Sorry, I'm not expert on C and hash_table, I'm open other suggestions
I can copy audiobridge->anncs into another hashtable changing the ref and while looping the new hashtable

}

/* Get rid of the announcements */
json_t *listAnncRemoved = json_array();
Copy link
Member

Choose a reason for hiding this comment

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

Code style: we don't use camel case for variables.


/* Get rid of the announcements */
json_t *listAnncRemoved = json_array();
for (GList *l = items_to_remove; l!= NULL; l = l->next) {
Copy link
Member

Choose a reason for hiding this comment

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

Code style: don't define variables in the for itself. Besides, please don't add an extra space between directive and bracket (it's for(, not for ().

for (GList *l = items_to_remove; l!= NULL; l = l->next) {
json_t *file_id = json_string(l->data);
if( g_hash_table_remove(audiobridge->anncs, l->data))
{
Copy link
Member

Choose a reason for hiding this comment

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

Code style: same for the if, there's an extra space that shouldn't be there before g_hash_table_remove.
The bracket of the check should be on the same line as the check, not below as you did here (if(..) {)

@lminiero
Copy link
Member

Did you mean to base this upon your "listannouncements" branch? I see changes coming from that PR too, in this branch.

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