[jitsi-dev] [jitsi/jitsi-videobridge] Fixes a problem where you may get stuck with low quality video (#225)


#1

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

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

-- Commit Summary --

  * Cosmetic changes.
  * Fixes the initial simulcast stream selection.

-- File Changes --

    M src/main/java/org/jitsi/videobridge/Endpoint.java (2)
    M src/main/java/org/jitsi/videobridge/simulcast/SimulcastSender.java (9)
    M src/main/java/org/jitsi/videobridge/simulcast/SimulcastSenderManager.java (32)

-- Patch Links --

https://github.com/jitsi/jitsi-videobridge/pull/225.patch
https://github.com/jitsi/jitsi-videobridge/pull/225.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/225


#2

LGTM

···

---
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/225#issuecomment-212483752


#3

@@ -134,8 +134,15 @@ public boolean accept(RawPacket pkt)
     private synchronized SimulcastSender getOrCreateSimulcastSender(
         SimulcastReceiver simulcastReceiver)
     {
- if (simulcastReceiver == null
- || !simulcastReceiver.isSimulcastSignaled())
+ if (simulcastReceiver == null)
+ {
+ return null;
+ }
+
+ SimulcastStream[] simulcastStreams
+ = simulcastReceiver.getSimulcastStreams();
+
+ if (simulcastStreams == null || simulcastStreams.length == 0)

What?! Could you please write a comment why you're checking for simulcastStreams? I get it that it's necessary later on but that's under 2 other ifs there so I don't think it's obvious why you need to perform the check and return early and in all cases regardless of the later ifs.

···

---
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/225/files/c7a83e1745869a0d9ff3a54ddfc6fad3150bd9c9#r60436026


#4

@@ -134,8 +134,15 @@ public boolean accept(RawPacket pkt)
     private synchronized SimulcastSender getOrCreateSimulcastSender(
         SimulcastReceiver simulcastReceiver)
     {
- if (simulcastReceiver == null
- || !simulcastReceiver.isSimulcastSignaled())
+ if (simulcastReceiver == null)
+ {
+ return null;
+ }
+
+ SimulcastStream[] simulcastStreams
+ = simulcastReceiver.getSimulcastStreams();
+
+ if (simulcastStreams == null || simulcastStreams.length == 0)

Thank you @lyubomir !

···

---
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/225/files/c7a83e1745869a0d9ff3a54ddfc6fad3150bd9c9#r60436956


#5

+ Endpoint sendingEndpoint = simulcastReceiver // never null.
+ .getSimulcastEngine() // never null.
+ .getVideoChannel() // never null.
+ .getEndpoint(); // might be null.
+
+ Endpoint receivingEndpoint = simulcastEngine // never null.
+ .getVideoChannel() // never null.
+ .getEndpoint(); // might be null.

(1) Could I please ask you to rewrite that without repeating field accesses, methods calls and comments? I believe the source code will become more readable. For example,

VideoChannel videoChannel = simulcastEngine.getVideoChannel();
int targetOrder = videoChannel.getReceiveSimulcastLayer();
...
Endpoint receivingEndpoint = videoChannel.getEndpoint();

(2) I find these comments about "never null" and "might be null" distracting and unnecessary at times. For example, you check for receivingEndpoing right afterwards so it's pointless to me to say here that it might be null because your source code says it.

Additionally, with the repeating field accesses and method calls, you get into inconsistencies like the first simulcastEngine access doesn't have a comment but later on it has a comment.

···

---
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/225/files/c7a83e1745869a0d9ff3a54ddfc6fad3150bd9c9#r60437167


#6

+ Endpoint sendingEndpoint = simulcastReceiver // never null.
+ .getSimulcastEngine() // never null.
+ .getVideoChannel() // never null.
+ .getEndpoint(); // might be null.
+
+ Endpoint receivingEndpoint = simulcastEngine // never null.
+ .getVideoChannel() // never null.
+ .getEndpoint(); // might be null.

Will do, thank you @lyubomir !

···

---
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/225/files/c7a83e1745869a0d9ff3a54ddfc6fad3150bd9c9#r60438637


#7

         {
+ // This is equivalent to !simulcastReceiver.isSimulcastSignaled() in

I don't want you to modify this any further, I merely want to throw in my brief observations:

You're in a private method which is called by a single (at the time of this writing) other method. The caller has the check:

(simulcastReceiver == null || !simulcastReceiver.isSimulcastSignaled())

And the caller appears to perform the check just because it wants to call this method, the caller doesn't need simulcastReceiver itself.

But then you have the same checks in this method:
(1) (simulcastReceiver == null)
(2) Another, more verbose alternative to !simulcastReceiver.isSimulcastSignaled().

Again, I don't want to modify this PR any further and I merely wanted to state that I think there's repetition which adds complexity and, consequently, increases maintenance costs.

···

---
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/225/files/6d820b18ef3bb196e410528985647e6ab221d714#r60443767


#8

Merged #225.

···

---
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/225#event-634830296