matrix-org / waterfall

A cascading stream forwarding unit for scalable, distributed voice and video conferencing over Matrix
Apache License 2.0
97 stars 5 forks source link

Add `unsubscribe` logic #79

Closed SimonBrandner closed 1 year ago

SimonBrandner commented 1 year ago

Fixes https://github.com/matrix-org/waterfall/issues/63

SimonBrandner commented 1 year ago

Looks good, but it looks like subscribe and unsubscribe functions in conference are essentially identical and only differ in the last step (one function calling subscribe function on peer, while another unsubscribes).

Could we possibly unify it so that we don't need to duplicate the code? If needed, we can also unify the function signature for subscribe and unsubscribe on a peer level (make subscribe accept a slice as the unsubscribe function does).

You're right - my initial implementation still left a lot to be desired. I think now, it might be even better than what we had before!

Also, to prevent problems, we probably need to take into account the possibility that subscribe and unsubscrib might contain the same track (e.g. an error made by the programmer on the other side) in which case the effect would probably be undesired. Ideally, we probably need to detect it and log the error (in the future also send an error to the client). What do you think?

Yeah, I wonder how/when we would want to check that since it's essentially O(m*n), so I would like to avoid blocking on it if we can. In theory, right now subscribe should take priority which makes sense from my POV and things shouldn't break, I think? So we perhaps add something to the end of the processTrackSubscriptionDCMessage() method, so that we don't block. What are your thoughts on that?