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

Make max_ccu based on users, not sessions #74

Merged
merged 1 commit into from
Feb 11, 2021
Merged

Make max_ccu based on users, not sessions #74

merged 1 commit into from
Feb 11, 2021

Conversation

mqp
Copy link
Contributor

@mqp mqp commented Feb 11, 2021

Fixes #57. (It was also off by one.)

@mqp mqp merged commit 500044f into master Feb 11, 2021
@mqp mqp deleted the fix-ccu branch February 11, 2021 04:27
return Err(From::from("Room is full."));
}
if switchboard.sessions().len() > config.max_ccu {
if switchboard.get_all_users().count() >= config.max_ccu {
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I saw in my tests this was off by one here, and forgot to report. 👍


// hack -- use data channel subscription to infer this, it would probably be nicer if
// connections announced explicitly whether they were a publisher or subscriber
let gets_data_channel = subscribe.as_ref().map(|s| s.data).unwrap_or(false);
let join_kind = if gets_data_channel { JoinKind::Publisher } else { JoinKind::Subscriber };

if join_kind == JoinKind::Publisher {
if switchboard.publishers_occupying(&room_id).len() > config.max_room_size {
if room_users.len() > config.max_room_size {
Copy link
Contributor

Choose a reason for hiding this comment

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

Previously we iterated twice over the publishers_by_room HashMap because get_users called publishers_occupying too. So this is nice we are only doing it once now.

result
pub fn get_room_users(&self, room: &RoomId) -> impl Iterator<Item = &UserId> {
self.publishers_occupying(room).iter().filter_map(|s| {
s.join_state.get().map(|j| &j.user_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

I was going to say what the behavior here when s.join_state.get() return null (create_session and setup_media have been called but not yet process_join), but publishers_occupying is using the publishers_by_room HashMap and we know that join_state is set at this time. So all good.

@vincentfretin
Copy link
Contributor

vincentfretin commented Feb 11, 2021

The max_ccu works now as documented in the config file, this is great.

A note, I see in the code there is process_subscribe to somewhat subscribe to a publisher without being a publisher yourself. This is something I'm interested in to have potentially a webinar with 1-5 publishers and 100 subscribers.
If a user is using process_subscribe, this user is not counted currently. They was somewhat counted before because we counted the sessions.

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.

max_ccu setting is misleading
2 participants