[jitsi-dev] [jitsi-videobridge] send a last-n notification when a client pins or n pins an endpoint (#187)


#1

the "last-n" notification is the source of truth for clients as to which streams they're receiving from the bridge. Therefore, it's best to rely on that as the trigger for local layout changes. If a client pins another client, it doesn't automatically get a new last-n notification which it would normally rely on to actually perform a switch. This change automatically sends an updated last-n notification when a pin/unpin occurs that changes the pinned list.
You can view, comment on, or merge this pull request online at:

  https://github.com/jitsi/jitsi-videobridge/pull/187

-- Commit Summary --

  * send a last-n notification when a client pins or n pins an endpoint

-- File Changes --

    M src/main/java/org/jitsi/videobridge/LastNController.java (1)

-- Patch Links --

https://github.com/jitsi/jitsi-videobridge/pull/187.patch
https://github.com/jitsi/jitsi-videobridge/pull/187.diff

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/187


#2

the "last-n" notification is the source of truth for clients as to which streams they're receiving from the bridge. Therefore, it's best to rely on that as the trigger for local layout changes.

Absolutely.

If a client pins another client, it doesn't automatically get a new last-n notification which it would normally rely on to actually perform a switch.

I think that it should get an update if and only if the list of forwarded endpoints changed as a result of the pinning. Do you see different behavior? Or do you think that an update is necessary even if the forwarded list doesn't change (e.g. the client pinned the current dominant speaker, and so the bridge continued to send the same streams)?

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/187#issuecomment-202514067


#3

I think that it should get an update if and only if the list of forwarded endpoints changed as a result of the pinning. Do you see different behavior? Or do you think that an update is necessary even if the forwarded list doesn't change (e.g. the client pinned the current dominant speaker, and so the bridge continued to send the same streams)?

Yes, we see that the notification does not get sent in this case. And agree, it should only be sent if the last-n message would be different (though a change in order would also count as different, yes?).

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/187#issuecomment-202516420


#4

Yes, we see that the notification does not get sent in this case. And agree, it should only be sent if the last-n message would be different (though a change in order would also count as different, yes?).

No, we don't consider the order. We do include an ordered list in the ["conferenceEndpoints" array](https://github.com/jitsi/jitsi-videobridge/blob/master/doc/last-n.md#client-side-events), but this is meant to give clients just entering the conference the necessary context (the history of speakers). From then on, clients can learn about the order of speakers by monitoring the changes to the current dominant speaker, sent in a separate event.

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/187#issuecomment-202523094


#5

I see. I remember the change you made to keep the list in order...I thought it was the last-n list, not the conferenceEndpoints array. So that field is always in order, but not necessarily last-n?

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/187#issuecomment-202560794


#6

Yes, this is a little confusing (ok, maybe not just a little :)).

The "lastNEndpoints" in the event corresponds to the "forwardedEndpoints" list in the code, which is meant to be a *set*. This is the actual set used when deciding whether to forward a video packet or not, and should be considered the source of truth.

The "conferenceEndpoints" array is the list of endpoints in the conference (possibly truncated), ordered by speech activity.

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/187#issuecomment-202566558


#7

Got it, clear now. Thanks Boris.

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/187#issuecomment-202568112


#8

@bgrozev just wanted to check in here, is there agreement that last-n notifications should be sent on pin/unpin (when things change, of course)? If so, from what I can tell, some change is needed (maybe not exactly as I have it here, let me know any feedback on what you'd like changed).

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/187#issuecomment-205443049


#9

@bgrozev just wanted to check in here, is there agreement that last-n notifications should be sent on pin/unpin (when things change, of course)?

Yes, provided the change modifies the set, and not just the order.

If so, from what I can tell, some change is needed (maybe not exactly as I have it here, let me know any feedback on what you'd like changed).

Can you describe the scenario in which you observe a problem? I don't spot anything wrong with the code which handles this, so I suppose it is in a special case I've missed.
https://github.com/jitsi/jitsi-videobridge/blob/master/src/main/java/org/jitsi/videobridge/LastNController.java#L517

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/187#issuecomment-214491520


#10

Possibly this is also fixed by #204 ?

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/187#issuecomment-214492468


#11

@bgrozev yes i think you're probably right. we can close this and if we see it again (after #204) i can re-open.

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/187#issuecomment-214501101


#12

Closed #187.

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/187#event-640546256


#13

Oops, just noticed your other comment. I can't remember off the top of my head as to where the hole in the logic was, and unfortunately i don't have any other notes here. but, like you said, with #204 in place then hopefully this won't be a problem anymore.

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/187#issuecomment-214502027