[jitsi-dev] [jitsi-videobridge] Simulcast fixes and optimizations. (#185)


#1

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

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

-- Commit Summary --

  * Takes into account RTP timestamp wrap around.
  * Syncs access to the sim. streams and their history.
  * Optimizes event firing.
  * Fixes formatting and removes an obsolete comment.
  * Deprecates the switching simulcast mode.
  * Fixes a sync issue in the rewriting send mode for simulcast.

-- File Changes --

    M src/main/java/org/jitsi/videobridge/simulcast/SimulcastReceiver.java (125)
    M src/main/java/org/jitsi/videobridge/simulcast/SimulcastSender.java (42)
    M src/main/java/org/jitsi/videobridge/simulcast/SimulcastStream.java (25)
    M src/main/java/org/jitsi/videobridge/simulcast/messages/SimulcastMessagesMapper.java (29)
    M src/main/java/org/jitsi/videobridge/simulcast/sendmodes/RewritingSendMode.java (99)
    M src/main/java/org/jitsi/videobridge/simulcast/sendmodes/SendMode.java (21)
    M src/main/java/org/jitsi/videobridge/simulcast/sendmodes/SwitchingSendMode.java (4)

-- Patch Links --

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


#2

     {
- this.simulcastStreams = simulcastStreams;
+ SimulcastStream[] oldSimulcastStreams = this.simulcastStreams;
+ int oldLen
+ = oldSimulcastStreams == null ? 0 : oldSimulcastStreams.length;
+ int newLen
+ = newSimulcastStreams == null ? 0 : newSimulcastStreams.length;
+
+ if (oldLen == newLen)

Can we simplify this? Maybe use something like:
boolean equal = (oldLen == 0 && newLen == 0) || Arrays.equals(oldSimulcastStreams, newSimulcastStreams);

···


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/185/files/6fb894a00a8e0fe144e5d6f151756ee17d4e9049..646aa9eaa575c1a29fe3077993944dcfe588f69f#r57361503


#3

@@ -571,9 +630,11 @@ private void maybeTimeout(
             SimulcastStream cause,
             RawPacket pkt,
             SimulcastStream effect,
+ List<SimulcastStream> localSimulcastStreamFrameHistory,

I think this (and endIndexInSimulcastStreamFrameHistory, too) needs a javadoc. It is not obvious what it is.

···


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/185/files/6fb894a00a8e0fe144e5d6f151756ee17d4e9049..646aa9eaa575c1a29fe3077993944dcfe588f69f#r57365318


#4

@@ -243,4 +243,29 @@ public void askForKeyframe()
     {
         simulcastReceiver.askForKeyframe(this);
     }
+
+ @Override

These need javadocs, and formatting fixes. I don't think we have one-line ifs anywhere else.

···


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/185/files/6fb894a00a8e0fe144e5d6f151756ee17d4e9049..646aa9eaa575c1a29fe3077993944dcfe588f69f#r57365479


#5

+ SimulcastStream that = (SimulcastStream) o;
+
+ if (primarySSRC != that.primarySSRC) return false;
+ if (rtxSSRC != that.rtxSSRC) return false;
+ if (fecSSRC != that.fecSSRC) return false;
+ return order == that.order;
+
+ }
+
+ @Override
+ public int hashCode()
+ {
+ int result = (int) (primarySSRC ^ (primarySSRC >>> 32));
+ result = 31 * result + (int) (rtxSSRC ^ (rtxSSRC >>> 32));
+ result = 31 * result + (int) (fecSSRC ^ (fecSSRC >>> 32));
+ result = 31 * result + order;

This makes me nervous. These fields should all be final if they are used in hashCode(). For primarySSRC, fecSSRC and order this can be made easily, the rtxSSRC will require a bit of refactoring.

···


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/185/files/6fb894a00a8e0fe144e5d6f151756ee17d4e9049..646aa9eaa575c1a29fe3077993944dcfe588f69f#r57366072


#6

@@ -562,6 +571,10 @@ else if (simStream == acceptedStream)
         }

Ideally we should split this method because it is way too long, but that's not related to the PR. It isn't obvious that changedStreams doesn't contain duplicates, and we probably want it to not have duplicates. I think we should use a Set here (after fixing SimulcastStream#hashCode)

···


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/185/files/646aa9eaa575c1a29fe3077993944dcfe588f69f..380edb69af2b0835ef870bc35b17ef118fc46f74#r57368982


#7

     {
- this.simulcastStreams = simulcastStreams;
+ SimulcastStream[] oldSimulcastStreams = this.simulcastStreams;
+ int oldLen
+ = oldSimulcastStreams == null ? 0 : oldSimulcastStreams.length;
+ int newLen
+ = newSimulcastStreams == null ? 0 : newSimulcastStreams.length;
+
+ if (oldLen == newLen)

Good point, thanks!

···


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/185/files/6fb894a00a8e0fe144e5d6f151756ee17d4e9049..646aa9eaa575c1a29fe3077993944dcfe588f69f#r57369264


#8

@@ -571,9 +630,11 @@ private void maybeTimeout(
             SimulcastStream cause,
             RawPacket pkt,
             SimulcastStream effect,
+ List<SimulcastStream> localSimulcastStreamFrameHistory,

Good point, thanks!

···


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/185/files/6fb894a00a8e0fe144e5d6f151756ee17d4e9049..646aa9eaa575c1a29fe3077993944dcfe588f69f#r57369311


#9

         }
     }

     /**
- * Gets the <tt>SimulcastStream</tt> that is currently being received.
- *
- * @return
+ * A simple class that holds the state of a {@RewritingSendMode}.

Typo?

···


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/185/files/30abf2867531669e08ec52bd59f19d90e0d45f4c..b39934049fcc4beea4568189f438a1ff02d1ceaa#r57370765


#10

- /**
- * Gets the <tt>SimulcastStream</tt> that was previously being received.
- *
- * @return
- */
- public SimulcastStream getNext()
- {
- WeakReference<SimulcastStream> wr = this.weakNext;
- return (wr != null) ? wr.get() : null;
+ /**
+ * A <tt>WeakReference</tt> to the <tt>SimulcastStream</tt> that is
+ * currently being received.
+ */
+ private final WeakReference<SimulcastStream> weakCurrent;

Since this is moved to a class of it's own, can we hide the WeakReference complexity with a getter and setter?

···


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/185/files/30abf2867531669e08ec52bd59f19d90e0d45f4c..b39934049fcc4beea4568189f438a1ff02d1ceaa#r57371010


#11

@@ -243,4 +243,29 @@ public void askForKeyframe()
     {
         simulcastReceiver.askForKeyframe(this);
     }
+
+ @Override

Good point, thanks!

···


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/185/files/6fb894a00a8e0fe144e5d6f151756ee17d4e9049..646aa9eaa575c1a29fe3077993944dcfe588f69f#r57374614


#12

+ SimulcastStream that = (SimulcastStream) o;
+
+ if (primarySSRC != that.primarySSRC) return false;
+ if (rtxSSRC != that.rtxSSRC) return false;
+ if (fecSSRC != that.fecSSRC) return false;
+ return order == that.order;
+
+ }
+
+ @Override
+ public int hashCode()
+ {
+ int result = (int) (primarySSRC ^ (primarySSRC >>> 32));
+ result = 31 * result + (int) (rtxSSRC ^ (rtxSSRC >>> 32));
+ result = 31 * result + (int) (fecSSRC ^ (fecSSRC >>> 32));
+ result = 31 * result + order;

Good point, thanks!

···


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/185/files/6fb894a00a8e0fe144e5d6f151756ee17d4e9049..646aa9eaa575c1a29fe3077993944dcfe588f69f#r57374637


#13

         }
     }

     /**
- * Gets the <tt>SimulcastStream</tt> that is currently being received.
- *
- * @return
+ * A simple class that holds the state of a {@RewritingSendMode}.

Yep :slight_smile:

···


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/185/files/30abf2867531669e08ec52bd59f19d90e0d45f4c..b39934049fcc4beea4568189f438a1ff02d1ceaa#r57374775


#14

Merged #185.

···

---
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/185#event-603083212