[jitsi-dev] [jitsi/jitsi-videobridge] Last-n notification broken in certain scenario (#192)


#1

The repro case I've been using is as follows:
1. Have last-n set to 2
2. Have endpoint A join a call
3. Have endpoint B join the same call. Media flows between A and B fine
4. Have endpoint C join the same call. Endpoints A and B will have one frozen video (due to #7 above, the last-n list will only contain the new conference endpoint, endpoint C)

From the code, what happens is:

1. speechActivityEndpointsChanged gets called with an ordered list of all endpoints in the conference
2. it calls speechActivityEndpointIdsChanged with that same list (just with the endpoint ids, not the entire endpoint object).
3. speechActivityEndpointIdsChanged filters out any 'existing' endpoints by calling removing anything from the current list that is already in conferenceSpeechActivityEndpoints
4. speechActivityEndpointIdsChanged calls update with that filtered list (just the new endpoints)
5. update calls "newForwardedEndpoints = orderBy(newConferenceEndpoints, conferenceSpeechActivityEndpoints);". Which means newForwardedEndpoints only contains endpoints that were in newConferenceEndpoints [the loss happens here...we're trimmed the list of forwarded endpoints to only what was in the newConferenceEndpoints]

The hack I've thrown together to fix this is to remove "newForwardedEndpoints = orderBy(newConferenceEndpoints, conferenceSpeechActivityEndpoints);" call in update, so we don't improperly trim the active speaker list. Not sure if that breaks other functionality.

cc @bgrozev

···

---
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/issues/192


#2

From the code, what happens is:
1. speechActivityEndpointsChanged gets called with an ordered list of all endpoints in the conference
2. it calls speechActivityEndpointIdsChanged with that same list (just with the endpoint ids, not the entire endpoint object).
3. speechActivityEndpointIdsChanged filters out any 'existing' endpoints by calling removing anything from the current list that is already in conferenceSpeechActivityEndpoints
4. speechActivityEndpointIdsChanged calls update with that filtered list (just the new endpoints)
5. update calls "newForwardedEndpoints = orderBy(newConferenceEndpoints, conferenceSpeechActivityEndpoints);". Which means newForwardedEndpoints only contains endpoints that were in newConferenceEndpoints [the loss happens here...we're trimmed the list of forwarded endpoints to only what was in the newConferenceEndpoints]

Could you give this branch a try? https://github.com/bgrozev/jitsi-videobridge/tree/last-n-fix

I *think* it's just this typo, because in step 4 above we update the conferenceSpeechActivityEndpoints field before calling update(), so it *should* work.

···

---
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/issues/192#issuecomment-203127820


#3

Hmmm, weird. I added an update here the other day but I guess it didn't go through. @bgrozev that looked good when I tried it out (using my test case which was repro'ing it every time).

···

---
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/issues/192#issuecomment-203670486


#4

Thanks, Brian! I saw your previous comment (on the commit). Creating a PR.

···

---
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/issues/192#issuecomment-203673396


#5

Closed #192 via ca2c9d0714af2bc5b9e741656179beea283ccf77.

···

---
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/issues/192#event-609031907