search

matrix-org / waterfall

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

Poor man's simulcast implementation #85

Closed daniel-abramov closed 1 year ago

daniel-abramov commented 1 year ago

Per-commit review is suggested (tried to keep the commit history clean and "atomic").

As discussed, I'm submitting this PR to simplify the review of the simulcast that would otherwise have been too overwhelming (over 1000 LOC PR).

This is stage 1 of simulcast implementation (fixes https://github.com/matrix-org/waterfall/issues/31).

daniel-abramov commented 1 year ago

I have to admit that some of the abstractions are becoming a bit unclear to me again (e.g. Subscription vs SubscribedTrack). I wonder if the naming could be improved here

We don't have a SubscribedTrack structure if I'm not mistaken 🤔, but we do have a Subscription which refers to a single subscription (subscription to a single track).

Or did you mean that you would prefer Subscription called SubscribedTrack? - I thought that it might be confusing because it would mean that SubscribedTrack encapsulates a track to which someone is subscribed (to me it feels like it gives a notion that there is one SubscribedTrack, i.e. if asdf is a track, to which someone is subscribed, does it have to be a type of SubscribedTrack and stored in a map, so that we can differentiate tracks to which someone is subscribed vs tracks that no-one is subscribed to).

Whereas with Subscription it's just a subscription (has a connotation that it is a new entity) to an existing published track, which means that there could be many of them, i.e. we may have 3,4 or 10 subscriptions to a single existing track.

SimonBrandner commented 1 year ago

I have to admit that some of the abstractions are becoming a bit unclear to me again (e.g. Subscription vs SubscribedTrack). I wonder if the naming could be improved here

We don't have a SubscribedTrack structure if I'm not mistaken thinking, but we do have a Subscription which refers to a single subscription (subscription to a single track).

Or did you mean that you would prefer Subscription called SubscribedTrack? - I thought that it might be confusing because it would mean that SubscribedTrack encapsulates a track to which someone is subscribed (to me it feels like it gives a notion that there is one SubscribedTrack, i.e. if asdf is a track, to which someone is subscribed, does it have to be a type of SubscribedTrack and stored in a map, so that we can differentiate tracks to which someone is subscribed vs tracks that no-one is subscribed to).

Whereas with Subscription it's just a subscription (has a connotation that it is a new entity) to an existing published track, which means that there could be many of them, i.e. we may have 3,4 or 10 subscriptions to a single existing track.

Ah, yes, sorry. The whole Subscription, Subscribers and TrackSubscribers has me confused...

daniel-abramov commented 1 year ago

LGTM

Do we wait to resolve the Firefox issue before merging?

@dbkr, do you want to have another look?

Yep, the issue with grey frames seems to be a bug, I need to fix it either prior to merging or after that.

UPD. Merging as agreed during the team sync-up.