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

Optimize notify_user to use get_publisher #80

Open
vincentfretin opened this issue Feb 16, 2021 · 3 comments
Open

Optimize notify_user to use get_publisher #80

vincentfretin opened this issue Feb 16, 2021 · 3 comments

Comments

@vincentfretin
Copy link
Contributor

process_block and process_unblock are using
notify_user(&event, &whom, switchboard.publishers_occupying(&joined.room_id));
so iterating over publishers in the rooms to find the session for the user id whom having notifications:true.
Looking at it for #55 when using multiple rooms, we should iterate in this case over all rooms and all sessions of those rooms to just find the session.
With an old version of the code, iterating over sessions of the room was better than iterating over all the sessions (this was what get_publisher was doing).
But we now have an optimized switchboard and get_publisher(user_id) is just O(1).
I think we can rewrite process_block and process_unblock and notify_user to use get_publisher here.
Note that previously we didn't use the same semantic for publisher, notify_user is searching for a session with notifications:true and in process_join we check for data:true.

I currently don't have the block/unblock feature in my app, so it may be a little bit difficult for me to test any changes, although I could test it in Chrome console and send the request manually.
I may try doing a PR myself in a month for this if I'm going to work on #55.

@vincentfretin
Copy link
Contributor Author

Note to myself, how to test block/unblock manually in the console.
With naf-janus-adapter example, connect with two users in the room.

> NAF.connection.adapter.availableOccupants
["0981198545288823"]
> NAF.clientId
"0701224502096699"
> let occupantToBlock = NAF.connection.adapter.availableOccupants[0]
verify you hear the other user
> NAF.connection.adapter.block(occupantToBlock)
you don't hear the other user
> NAF.connection.adapter.unblock(occupantToBlock)
you hear again the other user

vincentfretin added a commit to vincentfretin/janus-plugin-sfu that referenced this issue Jul 10, 2021
… publishers in the room to send the blocked/unblocked event (closes mozilla#80)
@vincentfretin
Copy link
Contributor Author

Note that previously we didn't use the same semantic for publisher, notify_user is searching for a session with notifications:true and in process_join we check for data:true.

Forget I said that. I misunderstood a little what the code was doing. The everyone list in notify_user is already only publishers in the room, here we are checking if the blocked user has subscribed for notifications just to know if they was blocked/unblocked by someone. So yes we still need to do this check, all good.

@vincentfretin
Copy link
Contributor Author

For completeness naf-janus-adapter is setting data and notifications to true for the publisher here: https://github.com/networked-aframe/naf-janus-adapter/blob/e0842fabdfa87e3a2291031f1f45643a208f7d23/src/index.js#L557-L558

vincentfretin added a commit to vincentfretin/janus-plugin-sfu that referenced this issue Jul 12, 2021
… publishers in the room to send the blocked/unblocked event (closes mozilla#80)
vincentfretin added a commit to networked-aframe/janus-plugin-sfu that referenced this issue Feb 12, 2022
… publishers in the room to send the blocked/unblocked event (closes mozilla#80)
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

No branches or pull requests

1 participant