[jitsi-dev] [jitsi-videobridge] Split last n (#122)


#1

You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Splits the last-n related code form VideoChannel into LastNController
  * Handles initialization properly.
  * Adds log messages.
  * Supports a list of pinned endpoints.
  * Does not handle pinned endpoints in simulcast.
  * Fixes the LastNController logger.
  * Sends last-N-related events via the DataChannel, avoids unnecessary updates, cleans up.
  * Avoids unnecessary keyframe requests. Initializes the conference endpoint list.
  * Avoids sending unnecessary keyframe requests.
  * Merge branch 'master' into split-last-n
  * Sends the list of lastNEndpoints when the data channel is opened.

-- File Changes --

    M src/main/java/org/jitsi/videobridge/Content.java (30)
    M src/main/java/org/jitsi/videobridge/Endpoint.java (205)
    A src/main/java/org/jitsi/videobridge/LastNController.java (538)
    M src/main/java/org/jitsi/videobridge/RtpChannel.java (33)
    M src/main/java/org/jitsi/videobridge/VideoChannel.java (740)
    M src/main/java/org/jitsi/videobridge/ratecontrol/BitrateController.java (15)
    M src/main/java/org/jitsi/videobridge/ratecontrol/VideoChannelLastNAdaptor.java (4)
    M src/main/java/org/jitsi/videobridge/simulcast/SimulcastSender.java (61)

-- Patch Links --

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

···

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


#2

@@ -210,7 +214,7 @@ public int calcNumEndpointsThatFitIn()
         long remainingBandwidth = availableBandwidth;
         int numEndpointsThatFitIn = 0;

- final Iterator<Endpoint> it = channel.getReceivingEndpoints();
+ final Iterator<Endpoint> it = null; //channel.getReceivingEndpoints();

Is the BitrateController actually used? If it is this would cause an NPE in a few in the while statement.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/122/files#r49116644


#3

         long now = System.currentTimeMillis();

         // The number of endpoints this channel is currently receiving
- int receivingEndpointCount = channel.getReceivingEndpointCount();
+ int receivingEndpointCount = 0;//channel.getReceivingEndpointCount();

Is this going to change in subsequent commits?

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/122/files#r49203887


#4

                 {
- this.weakPinnedEndpoint
- = new WeakReference<>(newPinnedEndpoint);
+ if (!pinnedEndpoints.get(i).

I could be misunderstanding, but can pinnedEndpoints.get(i) be null?

In our case, if you pin a stream then unpin the same stream (back to 0 pinnedEndpoints) this method will get called with a singleton list of [null].

It then seems like the resulting NPE here gets swallowed by someone and a Pinned Endpoint property event never fires.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/122/files#r50053222


#5

         }

- Conference conference = weakConference.get();
+ pinnedEndpointsChanged(Collections.singletonList(newPinnedEndpointID));

Here's how I fixed the bug mentioned in my previous comment:

List<String> newPinnedIDList = (newPinnedEndpointID == null) ? Collections.EMPTY_LIST : Collections.singletonList(newPinnedEndpointID);
        pinnedEndpointsChanged(newPinnedIDList);

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/122/files#r50053360


#6

Thanks, I'll incorporate the fix!

Personal opinion: I think using the list syntax ("pinnedEndpoints: []") even for a single endpoint is a better approach. It covers all cases, and I don't think we have any reason apart from backward compatibility to maintain the old one, so I would argue for deprecating it at some point.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/122#issuecomment-172906351


#7

Merged #122.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/122#event-522299221