search

mozilla / janus-plugin-sfu

Janus plugin to act as a kind of SFU for game networking data.
Mozilla Public License 2.0
135 stars 40 forks source link

Optimize notify_user to use get_publisher #80

Open vincentfretin opened 3 years ago

vincentfretin commented 3 years ago

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 https://github.com/mozilla/janus-plugin-sfu/issues/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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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