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

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

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@brianh5
Contributor

brianh5 commented Mar 24, 2016

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.

@bgrozev

This comment has been minimized.

Show comment
Hide comment
@bgrozev

bgrozev Mar 28, 2016

Member

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)?

Member

bgrozev commented Mar 28, 2016

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)?

@brianh5

This comment has been minimized.

Show comment
Hide comment
@brianh5

brianh5 Mar 28, 2016

Contributor

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?).

Contributor

brianh5 commented Mar 28, 2016

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?).

@bgrozev

This comment has been minimized.

Show comment
Hide comment
@bgrozev

bgrozev Mar 28, 2016

Member

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, 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.

Member

bgrozev commented Mar 28, 2016

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, 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.

@brianh5

This comment has been minimized.

Show comment
Hide comment
@brianh5

brianh5 Mar 28, 2016

Contributor

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?

Contributor

brianh5 commented Mar 28, 2016

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?

@bgrozev

This comment has been minimized.

Show comment
Hide comment
@bgrozev

bgrozev Mar 28, 2016

Member

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.

Member

bgrozev commented Mar 28, 2016

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.

@brianh5

This comment has been minimized.

Show comment
Hide comment
@brianh5

brianh5 Mar 28, 2016

Contributor

Got it, clear now. Thanks Boris.

Contributor

brianh5 commented Mar 28, 2016

Got it, clear now. Thanks Boris.

@brianh5

This comment has been minimized.

Show comment
Hide comment
@brianh5

brianh5 Apr 4, 2016

Contributor

@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).

Contributor

brianh5 commented Apr 4, 2016

@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).

@bgrozev

This comment has been minimized.

Show comment
Hide comment
@bgrozev

bgrozev Apr 25, 2016

Member

@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

Member

bgrozev commented Apr 25, 2016

@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

@bgrozev

This comment has been minimized.

Show comment
Hide comment
@bgrozev

bgrozev Apr 25, 2016

Member

Possibly this is also fixed by #204 ?

Member

bgrozev commented Apr 25, 2016

Possibly this is also fixed by #204 ?

@brianh5

This comment has been minimized.

Show comment
Hide comment
@brianh5

brianh5 Apr 25, 2016

Contributor

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

Contributor

brianh5 commented Apr 25, 2016

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

@brianh5 brianh5 closed this Apr 25, 2016

@brianh5

This comment has been minimized.

Show comment
Hide comment
@brianh5

brianh5 Apr 25, 2016

Contributor

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.

Contributor

brianh5 commented Apr 25, 2016

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.

@snhemanthm snhemanthm deleted the parlaylabs:last_n_notification_on_pin branch Dec 18, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment