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
@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
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
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
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
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
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
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
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
@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
@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
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
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
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
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
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