[jitsi-dev] [jitsi-videobridge] lastNEndpoints is initially empty even when there are endpoints in lastN (#121)


#1

I’ve noticed in running the videobridge that when a user joins a conference that is using last-n (non dynamic lastN), the lastNEndpoints list is empty when the initial message is sent on the data channel to the client. Even if there are other users already in the conference that are in lastN. The data channel message is triggered on line 840 of VideoChannel.java

@Override
    void sctpConnectionReady(Endpoint endpoint)
    {
        super.sctpConnectionReady(endpoint);

        if (endpoint.equals(getEndpoint()))
            lastNEndpointsChanged(null);
    }

This doesn’t seem like the intended behavior and may be a bug? I would expect the lastNEndpoints to be populated with the users already in lastN. Here is a message I captured from the videobridge

{"colibriClass":"LastNEndpointsChangeEvent","lastNEndpoints":[]}

This happens anytime a new client joins a conference, they get an empty lastNEndpoints list on initial join. If the dominant speaker then changes several times the clients get an updated message that has the right values for lastNEndpoints populated. This can cause the clients to not know who is actually in lastN.

I got around this behavior by changing sctpConnectionReady to call speechActivityEndpointsChanged(null) to populate the list

@Override
void sctpConnectionReady(Endpoint endpoint)
{
    super.sctpConnectionReady(endpoint);

    if (endpoint.equals(getEndpoint()))
        speechActivityEndpointsChanged(null);
}
If there is a better way to do this please let me know and I'll submit a fix. Additionally, it looks like the logic of building the lastNEndpoints could be factored out to a private method to avoid duplication between speechActivityEndpointsChanged() and setLastN(Integer lastN).

One further note, if an endpoint is in lastN, the list of lastNEndpoints does not include itself. For example, if endpoint1 and endpoint2 are in lastN and endpoint3 enters lastN due to a dominant speaker event endpoint1 will get a message like
{"colibriClass":"LastNEndpointsChangeEvent","lastNEndpoints":["endpoint2@nowhere.com","endpoint3@nowhere.com"],"endpointsEnteringLastN":["endpoint3@nowhere.com"]}

even though endpoint1 is still in lastN. It would be helpful to have itself in the list of lastNEndpoints so that the endpoint knows that it is still in lastN. Is there a specific reason that it is not included?

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/issues/121


#2

Hi David,

Thank you for the report. I agree that the initial message with empty lastN endpoints shouldn't be sent, and the bridge should instead send the actual list.

We are doing some refactoring of the lastN code here: https://github.com/jitsi/jitsi-videobridge/tree/split-last-n . A similar issue still exists there as well (I removed the code which sends the initial message, and haven't implemented it yet), but we'll fix it there shortly.

If you are experimenting with lastN, I suggest using this, because we plan to merge it soon (and it would help us find and fix any other issues sooner).

One further note, if an endpoint is in lastN, the list of lastNEndpoints does not include itself.

This is by design. The lastNEndpoints may be different for the different endpoints for a couple of reasons:
1. The value of "N" might vary from endpoint to endpoint (i.e. I may have lastN=1, while you have lastN=5). This may be either because of adaptation from the bridge (adaptive last-n), or because of restrictions from the client (I'm on a mobile, so my client requests to only see 1 video).
2. There may be pinned endpoints, which are also per-endpoint.

So it isn't clear when to include the local endpoint in the lastN list. I think keeping the semantics of lastNEndpoints as "the endpoints for which the bridge is currently forwarding video to you" is best.

Also, in your example (with the endpoints ordered by speech activity like so: endpoint3, endpoint1, endpoint2), endpoints 1 and 3 will receive lastNEndpoints with three entries, while endpoint2 will receive just two entries. They could remove themselves from the list, but I think that's more complicated and not very consistent.

Finally, if endpoints care about their position in the list ordered by speech activity, they should be able to find that out by monitoring DSI change events.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/issues/121#issuecomment-169630762


#3

@bgrozev, thank you for the reply. I will have a look at the split-last-n branch this morning and do some testing on it. What kind of timeframe are you looking at to have the code in for the initial message population? I can put in a hack in our local build until you have it ready.
On the lastNEndpoints not including its own endpoint, I can see your point in light of your explanation. Make sense to leave that as it is and work around our use case on the client side.
Thank you,David

···

On Thursday, January 7, 2016 6:02 AM, bgrozev <notifications@github.com> wrote:

Hi David,Thank you for the report. I agree that the initial message with empty lastN endpoints shouldn't be sent, and the bridge should instead send the actual list.We are doing some refactoring of the lastN code here: https://github.com/jitsi/jitsi-videobridge/tree/split-last-n . A similar issue still exists there as well (I removed the code which sends the initial message, and haven't implemented it yet), but we'll fix it there shortly.If you are experimenting with lastN, I suggest using this, because we plan to merge it soon (and it would help us find and fix any other issues sooner).
One further note, if an endpoint is in lastN, the list of lastNEndpoints does not include itself.
This is by design. The lastNEndpoints may be different for the different endpoints for a couple of reasons:
1. The value of "N" might vary from endpoint to endpoint (i.e. I may have lastN=1, while you have lastN=5). This may be either because of adaptation from the bridge (adaptive last-n), or because of restrictions from the client (I'm on a mobile, so my client requests to only see 1 video).
2. There may be pinned endpoints, which are also per-endpoint.So it isn't clear when to include the local endpoint in the lastN list. I think keeping the semantics of lastNEndpoints as "the endpoints for which the bridge is currently forwarding video to you" is best.Also, in your example (with the endpoints ordered by speech activity like so: endpoint3, endpoint1, endpoint2), endpoints 1 and 3 will receive lastNEndpoints with three entries, while endpoint2 will receive just two entries. They could remove themselves from the list, but I think that's more complicated and not very consistent.Finally, if endpoints care about their position in the list ordered by speech activity, they should be able to find that out by monitoring DSI change events.—
Reply to this email directly or view it on GitHub.

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/issues/121#issuecomment-169683123


#4

@bgrozev when you said: "or because of restrictions from the client (I'm on a mobile, so my client requests to only see 1 video)", how does a specific client request a certain number of streams? I thought the last-n sent to jicofo was conference wide?

Also are pinned video streams in addition to last-n or do they replace some of the last-n streams?

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/issues/121#issuecomment-169769776


#5

@bgrozev when you said: "or because of restrictions from the client (I'm on a mobile, so my client requests to only see 1 video)", how does a specific client request a certain number of streams? I thought the last-n sent to jicofo was conference wide?

With the current implementation a client cannot do this. But conceptually it should, and we have discussed adding it in the future (I don't know when, or in what way exactly).

Also are pinned video streams in addition to last-n or do they replace some of the last-n streams?

[They replace streams from lastN](https://github.com/jitsi/jitsi-videobridge/blob/split-last-n/src/main/java/org/jitsi/videobridge/LastNController.java#L390). That is, we first add the pinned endpoints, and then, if they are less than the lastN value, we fill from the rest (ordered by speech activity).

An interesting question is what to do when the client has pinned more endpoints than lastN allows for. Currently we forward all pinned nevertheless (see the comment in the code linked above).

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/issues/121#issuecomment-169776853


#6

David, I just pushed what I believe should be a fix in split-last-n. Note that it isn't tested yet.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/issues/121#issuecomment-169780142


#7

Untested code makes me giddy, I'll pull down the branch and run some tests. Thank you

···

On Jan 7, 2016, at 2:22 PM, bgrozev <notifications@github.com> wrote:

David, I just pushed what I believe should be a fix in split-last-n. Note that it isn't tested yet.


Reply to this email directly or view it on GitHub.

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/issues/121#issuecomment-169782901


#8

Yeah, it's going to get tested, of course, I just can't do it right now :slight_smile:
I figured I'd push early, so you can run it if you like.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/issues/121#issuecomment-169784117


#9

Much appreciated, I'll post my testing results

···

On Jan 7, 2016, at 2:38 PM, bgrozev <notifications@github.com> wrote:

Yeah, it's going to get tested, of course, I just can't do it right now :slight_smile:
I figured I'd push early, so you can run it if you like.


Reply to this email directly or view it on GitHub.

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/issues/121#issuecomment-169788482


#10

I did some testing with 4 endpoints and a last N value of 2. Everything looks good with the latest version of code. Messages are being sent as expected on the data channel and the initial lastNEndpoints message has a populated list. Thank you for the quick fix. I will keep an eye on this branch for when it gets merged and close this ticket

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/issues/121#issuecomment-170019107


#11

Closed #121.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/issues/121#event-523593312


#12

The split-last-n branch is now merged.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/issues/121#issuecomment-174093508