[jitsi-dev] [jitsi/jitsi-videobridge] Rewriting refactoring (#316)


#1

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

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

-- Commit Summary --

  * refactor(concurrency): RecurringProcessible -> RecurringRunnable .
  * ref(sim): Removes unsused import.
  * ref(rtcp): Removes obsolete RTCP termination strategy.
  * ref(rewriting): Sanitizes SSRC rewriting.

-- File Changes --

    M src/main/java/org/jitsi/videobridge/RtpChannel.java (131)
    M src/main/java/org/jitsi/videobridge/VideoChannel.java (226)
    M src/main/java/org/jitsi/videobridge/Videobridge.java (11)
    M src/main/java/org/jitsi/videobridge/eventadmin/callstats/CallStatsConferenceStatsHandler.java (52)
    M src/main/java/org/jitsi/videobridge/ratecontrol/AdaptiveSimulcastBitrateController.java (28)
    M src/main/java/org/jitsi/videobridge/ratecontrol/LastNBitrateController.java (32)
    D src/main/java/org/jitsi/videobridge/rtcp/FallbackingRTCPTerminationStrategy.java (238)
    D src/main/java/org/jitsi/videobridge/rtcp/VideoChannelRTCPTerminationStrategy.java (63)
    M src/main/java/org/jitsi/videobridge/simulcast/SimulcastStream.java (1)
    M src/main/java/org/jitsi/videobridge/stats/StatsManager.java (68)
    M src/test/java/org/jitsi/videobridge/simulcast/SimulcastReceiverTest.java (12)
    M src/test/java/org/jitsi/videobridge/simulcast/SimulcastTest.java (16)

-- Patch Links --

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


#2

@gpolitis pushed 3 commits.

8a55f27 RecurringRunnablesExecutor -> RecurringRunnableExecutor
5417ea1 ref(concurrency): Renames *process -> *run.
2025eaf ref(media): Simplifies the media stream management.

···


You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/316/files/8e169bd5eb1f35441f6614d78536ed8c2090dc19..2025eaf76b570628627bdf46b0943cdd491e4582


#3

lyubomir commented on this pull request.

+ long[] encoding = encodings.get(entry.getKey());

+ track.addEncoding(encoding[0], encoding[2], -1, (int) encoding[1]);
+ }

···

+
+ Map<Long, MediaStreamTrack> remoteTracks = stream.getRemoteTracks();
+
+ synchronized (remoteTracks)
+ {
+ remoteTracks.clear();
+ remoteTracks.putAll(tracksBySSRC);
+
+ for (Long primarySSRC : freeSSRCs)
+ {
+ MediaStreamTrack mst = new MediaStreamTrack();
+ mst.addEncoding(primarySSRC, -1, -1, RTPEncoding.BASE_ORDER);
+ remoteTracks.put(primarySSRC, mst);

That's murky business. I saw that you exposed the private field/state of `MediaStreamImpl` through `#getRemoteTracks()` and I let it be because I didn't want to stop your libjisti PR. However, this is a bit too much for me to swallow without mentioning it at least. You get the Map from `stream` without being 100% sure as a client that the implementation is not a copy of the private state, then you synchronize on it, and then you go an modify it hoping that you're modifying the private state of `MediaStreamImpl`!

--
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/316#pullrequestreview-2376913


#4

lyubomir commented on this pull request.

+ int order = RTPEncoding.BASE_ORDER;

+ for (SourcePacketExtension spe : sgpe.getSources())
+ {
+ long primarySSRC = spe.getSSRC();
+ freeSSRCs.remove(primarySSRC);

···

+
+ long[] encoding = new long[] { -1, -1, -1 };
+ encoding[0] = primarySSRC;
+ encoding[1] = order++;
+
+ encodings.put(primarySSRC, encoding);
+ tracksBySSRC.put(primarySSRC, track);
+ }
+ }
+ else if ("fid".equalsIgnoreCase(sgpe.getSemantics())
+ && sgpe.getSources() != null

`sgpe.getSources() != null` is checked in both `if`s. That's an unnecessary both as a runtime operation and as source code to be read.

--
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/316#pullrequestreview-2377549


#5

lyubomir commented on this pull request.

+

+ long[] encoding = new long[] { -1, -1, -1 };
+ encoding[0] = primarySSRC;
+ encoding[1] = order++;

···

+
+ encodings.put(primarySSRC, encoding);
+ tracksBySSRC.put(primarySSRC, track);
+ }
+ }
+ else if ("fid".equalsIgnoreCase(sgpe.getSemantics())
+ && sgpe.getSources() != null
+ && sgpe.getSources().size() == 2)
+ {
+ Long primarySSRC = sgpe.getSources().get(0).getSSRC();
+ freeSSRCs.remove(primarySSRC);
+ Long rtxSSRC = sgpe.getSources().get(1).getSSRC();

I counted 6 occurrences of `sgpe.getSources()` in this loop. I believe a local variable will make the source code easier to read.

--
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/316#pullrequestreview-2377770


#6

lyubomir commented on this pull request.

+ && sgpe.getSources() != null

+ && sgpe.getSources().size() >= 2)
+ {
+ // Every simulcast group determines an mst with a bunch of
+ // encodings.
+ MediaStreamTrack track = new MediaStreamTrack();

···

+
+ int order = RTPEncoding.BASE_ORDER;
+ for (SourcePacketExtension spe : sgpe.getSources())
+ {
+ long primarySSRC = spe.getSSRC();
+ freeSSRCs.remove(primarySSRC);
+
+ long[] encoding = new long[] { -1, -1, -1 };
+ encoding[0] = primarySSRC;
+ encoding[1] = order++;

Ugh, doesn't Java allow you to do the initialization without constants? For example,

long[] encoding = new long[] { primarySSRC, order++, -1 };

I mean, there are magic indices, `-1` that get immediately overwritten.... Even the following would be better

long[] encoding = new long[3];
encoding[0] = primarySSRC;
encoding[1] = order++;
encoding[2] = -1;

--
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/316#pullrequestreview-2378701


#7

lyubomir commented on this pull request.

+ {

+ if ((sources == null || sources.isEmpty())
+ && (sourceGroups == null || sourceGroups.isEmpty()))
+ {
+ return;
+ }

···

+
+ this.setSources(sources); // TODO remove and rely on MSTs.
+ this.setSourceGroups(sourceGroups); // TODO remove and rely on MSTs.
+
+ Map<Long, MediaStreamTrack> tracksBySSRC = new TreeMap<>();
+ Map<Long, long[]> encodings = new TreeMap<>();
+
+ List<Long> freeSSRCs = new ArrayList<>();
+
+ if (sources != null && sources.size() != 0)

I find `!sources.isEmpty()` clearer than `sources.size() != 0`.

--
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/316#pullrequestreview-2379045


#8

lyubomir commented on this pull request.

+ {

+ if ((sources == null || sources.isEmpty())
+ && (sourceGroups == null || sourceGroups.isEmpty()))
+ {
+ return;
+ }

···

+
+ this.setSources(sources); // TODO remove and rely on MSTs.
+ this.setSourceGroups(sourceGroups); // TODO remove and rely on MSTs.
+
+ Map<Long, MediaStreamTrack> tracksBySSRC = new TreeMap<>();
+ Map<Long, long[]> encodings = new TreeMap<>();
+
+ List<Long> freeSSRCs = new ArrayList<>();
+
+ if (sources != null && sources.size() != 0)

And why do we have to write these checks twice. Cannot we write them once and use local variables?

--
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/316#pullrequestreview-2379161


#9

gpolitis commented on this pull request.

+ long[] encoding = encodings.get(entry.getKey());

+ track.addEncoding(encoding[0], encoding[2], -1, (int) encoding[1]);
+ }

···

+
+ Map<Long, MediaStreamTrack> remoteTracks = stream.getRemoteTracks();
+
+ synchronized (remoteTracks)
+ {
+ remoteTracks.clear();
+ remoteTracks.putAll(tracksBySSRC);
+
+ for (Long primarySSRC : freeSSRCs)
+ {
+ MediaStreamTrack mst = new MediaStreamTrack();
+ mst.addEncoding(primarySSRC, -1, -1, RTPEncoding.BASE_ORDER);
+ remoteTracks.put(primarySSRC, mst);

Thank you Lyubo. The idea here is that the signaling manages the map, as you suggested in the LJ pr. But how would you like to change this?

--
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/316


#10

@gpolitis pushed 2 commits.

8f9f819 style(signaling): Prevents unnecessary calls to method.
e17d61a style(signaling): More readable array initialization.

···


You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/316/files/2025eaf76b570628627bdf46b0943cdd491e4582..e17d61abe063a4e6247fc5d624b240dc3881b3f6


#11

@gpolitis pushed 1 commit.

98edfcf style(signaling): Prevents unnecessary method calls.

···


You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/316/files/e17d61abe063a4e6247fc5d624b240dc3881b3f6..98edfcff0d893eb73e773262d5378e5064e7661b


#12

gpolitis commented on this pull request.

+ int order = RTPEncoding.BASE_ORDER;

+ for (SourcePacketExtension spe : sgpe.getSources())
+ {
+ long primarySSRC = spe.getSSRC();
+ freeSSRCs.remove(primarySSRC);

···

+
+ long[] encoding = new long[] { -1, -1, -1 };
+ encoding[0] = primarySSRC;
+ encoding[1] = order++;
+
+ encodings.put(primarySSRC, encoding);
+ tracksBySSRC.put(primarySSRC, track);
+ }
+ }
+ else if ("fid".equalsIgnoreCase(sgpe.getSemantics())
+ && sgpe.getSources() != null

Fixed, thank you Lyubo!

--
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/316


#13

gpolitis commented on this pull request.

+

+ long[] encoding = new long[] { -1, -1, -1 };
+ encoding[0] = primarySSRC;
+ encoding[1] = order++;

···

+
+ encodings.put(primarySSRC, encoding);
+ tracksBySSRC.put(primarySSRC, track);
+ }
+ }
+ else if ("fid".equalsIgnoreCase(sgpe.getSemantics())
+ && sgpe.getSources() != null
+ && sgpe.getSources().size() == 2)
+ {
+ Long primarySSRC = sgpe.getSources().get(0).getSSRC();
+ freeSSRCs.remove(primarySSRC);
+ Long rtxSSRC = sgpe.getSources().get(1).getSSRC();

Fixed, thank you Lyubo!

--
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/316


#14

gpolitis commented on this pull request.

+ && sgpe.getSources() != null

+ && sgpe.getSources().size() >= 2)
+ {
+ // Every simulcast group determines an mst with a bunch of
+ // encodings.
+ MediaStreamTrack track = new MediaStreamTrack();

···

+
+ int order = RTPEncoding.BASE_ORDER;
+ for (SourcePacketExtension spe : sgpe.getSources())
+ {
+ long primarySSRC = spe.getSSRC();
+ freeSSRCs.remove(primarySSRC);
+
+ long[] encoding = new long[] { -1, -1, -1 };
+ encoding[0] = primarySSRC;
+ encoding[1] = order++;

Fixed, thank you Lyubo!

--
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/316


#15

gpolitis commented on this pull request.

+ {

+ if ((sources == null || sources.isEmpty())
+ && (sourceGroups == null || sourceGroups.isEmpty()))
+ {
+ return;
+ }

···

+
+ this.setSources(sources); // TODO remove and rely on MSTs.
+ this.setSourceGroups(sourceGroups); // TODO remove and rely on MSTs.
+
+ Map<Long, MediaStreamTrack> tracksBySSRC = new TreeMap<>();
+ Map<Long, long[]> encodings = new TreeMap<>();
+
+ List<Long> freeSSRCs = new ArrayList<>();
+
+ if (sources != null && sources.size() != 0)

Fixed, thank you Lyubo!

--
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/316


#16

gpolitis commented on this pull request.

+ {

+ if ((sources == null || sources.isEmpty())
+ && (sourceGroups == null || sourceGroups.isEmpty()))
+ {
+ return;
+ }

···

+
+ this.setSources(sources); // TODO remove and rely on MSTs.
+ this.setSourceGroups(sourceGroups); // TODO remove and rely on MSTs.
+
+ Map<Long, MediaStreamTrack> tracksBySSRC = new TreeMap<>();
+ Map<Long, long[]> encodings = new TreeMap<>();
+
+ List<Long> freeSSRCs = new ArrayList<>();
+
+ if (sources != null && sources.size() != 0)

Fixed, thank you Lyubo!

--
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/316


#17

Closed #316.

···

--
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/316#event-809377691