[jitsi-dev] [jitsi/jitsi-videobridge] Sync fixes (#301)


#1

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

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

-- Commit Summary --

  * fix: Synchronize access to #endpoints.
  * fix: Synchronizes access to #fidSourceGroups.
  * fix: Uses a ConcurrentHashMap instead of a HashMap.
  * fix: Uses a ConcurrentHashMap instead of a HashMap.
  * fix: Adds synchronization.

-- File Changes --

    M src/main/java/org/jitsi/videobridge/EndpointRecorder.java (39)
    M src/main/java/org/jitsi/videobridge/RtpChannel.java (47)
    M src/main/java/org/jitsi/videobridge/SctpConnection.java (8)
    M src/main/java/org/jitsi/videobridge/VideoChannel.java (30)
    M src/main/java/org/jitsi/videobridge/pubsub/PubSubPublisher.java (12)
    M src/main/java/org/jitsi/videobridge/ratecontrol/LastNBitrateController.java (4)

-- Patch Links --

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


#2

@@ -153,7 +153,8 @@ private static synchronized int generateDebugId()
     /**
      * Data channels mapped by SCTP stream identified(sid).
      */
- private final Map<Integer,WebRtcDataStream> channels = new HashMap<>();
+ private final Map<Integer,WebRtcDataStream> channels
+ = new ConcurrentHashMap<>();

I see in the source code of SctpConnection that the access to the field channels is synchronized on the field syncRoot. Did you see an inconsistency that prompted the necessity of ConcurrentHashMap?

···

--
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/301/files/5914475813d212d8edc2931e92185b38e70dcaef#r77630631


#3

@@ -573,7 +573,7 @@ private void add(long time, long rate)
         /**
          * Removes values added before <tt>time - period</tt>.
          */
- private void clean(long time)
+ private synchronized void clean(long time)

The synchronization seems inconsistent to me because the method clean modifies both receivedRembs.size() and sum which are both read from the unsynchronized method getAverage.

On a side note, I find the implementation of the clean method to be inefficient and more complex than really necessary:
- toRemove is an unnecessary collection that takes up instance space, grows and shrinks;
- toRemove retains elements even after it has lived up its purpose;
- Because of toRemove, more auto(un)boxing happens;
- receivedRembs is searched upon removal.
I believe a solution based on an Iterator over receivedRembs.entrySet() will be more efficient and the source code will be 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/301/files/5914475813d212d8edc2931e92185b38e70dcaef#r77633809


#4

@@ -153,7 +153,8 @@ private static synchronized int generateDebugId()
     /**
      * Data channels mapped by SCTP stream identified(sid).
      */
- private final Map<Integer,WebRtcDataStream> channels = new HashMap<>();
+ private final Map<Integer,WebRtcDataStream> channels
+ = new ConcurrentHashMap<>();

No, removing the commit (or rather, not including it in the new version of the PR).

···

--
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/301/files/5914475813d212d8edc2931e92185b38e70dcaef#r77675450


#5

@@ -573,7 +573,7 @@ private void add(long time, long rate)
         /**
          * Removes values added before <tt>time - period</tt>.
          */
- private void clean(long time)
+ private synchronized void clean(long time)

Thanks Lyubo! I don't consider the implementation of this class complete. I'll add your specific notes as comments in the code, to be addressed when we next work on it.

···

--
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/301/files/5914475813d212d8edc2931e92185b38e70dcaef#r77676561


#6

Moved to #305

···

--
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/301#issuecomment-245046680


#7

Closed #301.

···

--
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/301#event-779792381