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.

Sign up for GitHub

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

Jump to bottom

AudioBridge stop_all_files api #3403

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

AudioBridge stop_all_files api #3403

wants to merge 2 commits into from

Conversation

keremcadirci
Copy link
Contributor

@keremcadirci keremcadirci commented Jul 8, 2024 •
edited
Loading

The continuation of the PR #3392
This is a cleaner PR with all #3392 commits are merged into a single commit.
All Conflicts are solved

@keremcadirci keremcadirci mentioned this pull request Jul 8, 2024
Closed
lminiero
lminiero reviewed Sep 6, 2024
View reviewed changes
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.

Apologies for the late reply, I'm catching up with pending activities after the summer holidays. I've added a couple of notes inline.

src/plugins/janus_audiobridge.c Outdated Show resolved Hide resolved
src/plugins/janus_audiobridge.c
json_object_set_new(event, "room",
string_ids ? json_string(audiobridge->room_id_str) : json_integer(audiobridge->room_id));
json_object_set_new(event, "file_id", json_string(p->annc->id));
janus_audiobridge_notify_participants(p, event, TRUE);
Copy link
Member

Choose a reason for hiding this comment

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

Why are you passing TRUE here? That's notify_source_participant, which should be FALSE in this case because the janus_audiobridge_participant instance you're passing to the function is an announcement, and so not a real participant with a Janus API session we can notify about stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @lminiero

How should we continue? Should we change stop_file and play_file to FALSE too

My own preference is to leave TRUE since in we want to notify participants about these events

src/plugins/janus_audiobridge.c Outdated Show resolved Hide resolved
keremcadirci added a commit to keremcadirci/janus-gateway that referenced this pull request Nov 5, 2024
@keremcadirci
Merge pull request #11 from keremcadirci/AudioBridge_Stop-All-Annc
6752dae 
meetecho#3403 stop_all_files, refactor
@keremcadirci
Copy link
Contributor Author

@lminiero
The 2 conversation above are resolved in the fallowing commit:
bb753c5

Waiting your comments on the unresolved conversation

I have tested the commit with multiple simultaneous play_file and later stop_all, it seems OK

@keremcadirci keremcadirci requested a review from lminiero November 5, 2024 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lminiero lminiero Awaiting requested review from lminiero

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@keremcadirci @lminiero