[jitsi-dev] [jitsi/libjitsi] Sync fixes (#196)


#1

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

  https://github.com/jitsi/libjitsi/pull/196

-- Commit Summary --

  * fix: Uses ConcurrentHashMap-s instead of HashMap-s.
  * fix: Uses a ConcurrentHashMap instead of a HashMap.
  * fix: Uses a ConcurrentHashMap instead of a HashMap.
  * style: Adds braces.
  * fix: Uses a ConcurrentHashMap instead of a HashMap.
  * fix: Uses a ConcurrentHashMap instead of a HashMap.
  * fix: Fixes synchronization issues.
  * fix: Fixes synchronization issues.
  * fix: Synchronize access to ssrcToRewriter.
  * fix: Uses a ConcurrentHashMap instead of a HashMap.
  * fix: Synchronize access to ssrcToRewriter.
  * fix: Fixes synchronization issues.
  * fix: Uses a ConcurrentHashMap instead of a HashMap.
  * fix: Uses a ConcurrentHashMap instead of a HashMap.
  * fix: Fixes synchronization issues.

-- File Changes --

    M src/org/jitsi/impl/configuration/ConfigurationServiceImpl.java (7)
    M src/org/jitsi/impl/neomedia/MediaServiceImpl.java (3)
    M src/org/jitsi/impl/neomedia/MediaUtils.java (13)
    M src/org/jitsi/impl/neomedia/format/MediaFormatImpl.java (7)
    M src/org/jitsi/impl/neomedia/format/ParameterizedVideoFormat.java (9)
    M src/org/jitsi/impl/neomedia/recording/PacketBuffer.java (29)
    M src/org/jitsi/impl/neomedia/recording/SynchronizerImpl.java (53)
    M src/org/jitsi/impl/neomedia/rtp/StreamRTPManager.java (20)
    M src/org/jitsi/impl/neomedia/rtp/sendsidebandwidthestimation/BandwidthEstimatorImpl.java (3)
    M src/org/jitsi/impl/neomedia/transform/DiscardTransformEngine.java (19)
    M src/org/jitsi/impl/neomedia/transform/fec/FECTransformEngine.java (18)
    M src/org/jitsi/impl/neomedia/transform/rewriting/SsrcGroupRewriter.java (5)
    M src/org/jitsi/impl/neomedia/transform/rewriting/SsrcRewritingEngine.java (2)
    M src/org/jitsi/impl/neomedia/transform/rtcp/StatisticsEngine.java (36)

-- Patch Links --

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


#2

@@ -102,7 +103,8 @@
      * and attempts to do so will simply be ignored.
      * @see #defaultProperties
      */
- private Map<String, String> immutableDefaultProperties = new HashMap<>();
+ private Map<String, String> immutableDefaultProperties
+ = new ConcurrentHashMap<>();

That seems unnecessary because immutableDefaultProperties is modified only during the construction of ConfigurationServiceImpl.

···

--
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/libjitsi/pull/196/files/36aed379244c7a28d7d30439da350121c72e3e25#r77404586


#3

@@ -111,7 +113,8 @@
      * <tt>setProperty()</tt> methods. Still, re-setting one of these properties
      * to <tt>null</tt> would cause for its initial value to be restored.
      */
- private Map<String, String> defaultProperties = new HashMap<>();
+ private Map<String, String> defaultProperties
+ = new ConcurrentHashMap<>();

Like immutableDefaultProperties, defaultProperties appears to be modified only during the construction of ConfiguratinoServiceImpl and, consequently, it seems unnecessary to use a 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/libjitsi/pull/196/files/36aed379244c7a28d7d30439da350121c72e3e25#r77404940


#4

@@ -53,7 +54,7 @@
      * defined in RFC 3551.
      */
     private static final Map<String, String> jmfEncodingToEncodings
- = new HashMap<String, String>();
+ = new ConcurrentHashMap<>();

That seems unnecessary because jmfEncodingToEncodings is modified only during the static constructor of MediaUtils.

···

--
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/libjitsi/pull/196/files/36aed379244c7a28d7d30439da350121c72e3e25#r77405487


#5

@@ -86,7 +87,7 @@
      */
     private static final Map<String, MediaFormat[]>
         rtpPayloadTypeStrToMediaFormats
- = new HashMap<String, MediaFormat[]>();
+ = new ConcurrentHashMap<>();

Like jmfEncodingToEncodings, rtpPayloadTypeStrToMediaFormats appears to be modified only during the static constructor of MediaUtils and, consequently, it seems unnecessary to use a 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/libjitsi/pull/196/files/36aed379244c7a28d7d30439da350121c72e3e25#r77405762


#6

         for (MediaFormat mediaFormat : rtpPayloadTypelessMediaFormats)
+ {
             if (MediaType.AUDIO.equals(mediaFormat.getMediaType()))
                 audioMediaFormats.add(mediaFormat);

It looks inconsistent to me to add all {} in the previous loop and not add all in the next loop.

···

--
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/libjitsi/pull/196/files/36aed379244c7a28d7d30439da350121c72e3e25#r77405927


#7

@@ -255,11 +256,11 @@ protected MediaFormatImpl(
         this.formatParameters
             = ((formatParameters == null) || formatParameters.isEmpty())
                 ? EMPTY_FORMAT_PARAMETERS
- : new HashMap<String, String>(formatParameters);
+ : new ConcurrentHashMap<>(formatParameters);

That seems unnecessary because the value of the fieldParameters is never modified in and exposed out of MediaFormatImpl.

···

--
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/libjitsi/pull/196/files/36aed379244c7a28d7d30439da350121c72e3e25#r77406196


#8

         this.advancedAttributes
             = ((advancedAttributes == null) || advancedAttributes.isEmpty())
                 ? EMPTY_FORMAT_PARAMETERS
- : new HashMap<String, String>(advancedAttributes);
+ : new ConcurrentHashMap<>(advancedAttributes);

The comment on formatParameters applies to advancedAttributes as well.

···

--
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/libjitsi/pull/196/files/36aed379244c7a28d7d30439da350121c72e3e25#r77406298


#9

@@ -64,7 +65,7 @@ public ParameterizedVideoFormat(
         this.fmtps
             = ((fmtps == null) || fmtps.isEmpty())
                 ? MediaFormatImpl.EMPTY_FORMAT_PARAMETERS
- : new HashMap<String, String>(fmtps);
+ : new ConcurrentHashMap<>(fmtps);

The Map instance stored in the field fmtps is never modified in and exposed out of ParameterizedVideoFormat.

···

--
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/libjitsi/pull/196/files/36aed379244c7a28d7d30439da350121c72e3e25#r77406481


#10

@@ -145,7 +146,7 @@ protected void copy(Format f)
             fmtps
                 = ((pvfFmtps == null) || pvfFmtps.isEmpty())
                     ? MediaFormatImpl.EMPTY_FORMAT_PARAMETERS
- : new HashMap<String, String>(pvfFmtps);
+ : new ConcurrentHashMap<>(pvfFmtps);

The same as above. The use of ConcurrentHashMap is unnecessary.

···

--
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/libjitsi/pull/196/files/36aed379244c7a28d7d30439da350121c72e3e25#r77406550


#11

         {
- synchronized (ssrcs)
+ SSRCDesc ssrcDesc = ssrcs.get(ssrc);

I'm afraid I don't get it. In removeMapping you protected the read with a synchronized so I guess you want reads to be protected. But then you left the read in getLocalTime unprotected so I'm confused now about what you want.

···

--
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/libjitsi/pull/196/files/36aed379244c7a28d7d30439da350121c72e3e25#r77409612


#12

         {
- synchronized (endpoints)
+ Endpoint endpoint = endpoints.get(endpointId);

Unlike ssrcs, there's no protected block that does only reading so I don't think that you want the reads protected. Anyway, I'd like to mention getLocalTime again because there's an unprotected read from endpoints. Some analysis tools would flag such a synchronization as inconsistent.

···

--
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/libjitsi/pull/196/files/36aed379244c7a28d7d30439da350121c72e3e25#r77410025


#13

@@ -102,7 +103,8 @@
      * and attempts to do so will simply be ignored.
      * @see #defaultProperties
      */
- private Map<String, String> immutableDefaultProperties = new HashMap<>();
+ private Map<String, String> immutableDefaultProperties
+ = new ConcurrentHashMap<>();

Thanks, Lyubo! I was trying to stay on the safe side and did not always follow all the usages. I'll remove the commits that change ConfigurationServiceImpl, MediaServiceImpl, MediaFormatImpl and ParameterizedVideoFormat.

···

--
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/libjitsi/pull/196/files/36aed379244c7a28d7d30439da350121c72e3e25#r77411733


#14

         {
- synchronized (ssrcs)
+ SSRCDesc ssrcDesc = ssrcs.get(ssrc);

You are right, the read doesn't need to be protected here. I simply refactored the existing code. Will remove 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/libjitsi/pull/196/files/36aed379244c7a28d7d30439da350121c72e3e25#r77412273


#15

@@ -168,14 +171,17 @@ public void putResumableStreamRewriter(
     public ResumableStreamRewriter getResumableStreamRewriter(Long ssrc,
                                                               boolean create)
     {
- ResumableStreamRewriter rewriter = ssrcToRewriter.get(ssrc);
- if (rewriter == null && create)
+ synchronized (ssrcToRewriter)

I'm pretty sure there's only a single thread accessing the map. The documentation is not clear about, sorry about that. Would you like me to add a note explaining this and why it is the case?

···

--
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/libjitsi/pull/196/files/36aed379244c7a28d7d30439da350121c72e3e25#r77412805


#16

         {
- synchronized (endpoints)
+ Endpoint endpoint = endpoints.get(endpointId);

Yeah, I'm fine with those unprotected reads. Protecting the single read of "ssrcs" was a mistake.

···

--
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/libjitsi/pull/196/files/36aed379244c7a28d7d30439da350121c72e3e25#r77412841


#17

@@ -66,7 +68,8 @@
      * A map of SSRCs to <tt>SsrcRewriter</tt>. Each SSRC that we rewrite in
      * this group rewriter has its own rewriter.
      */
- private final Map<Integer, SsrcRewriter> rewriters = new HashMap<>();
+ private final Map<Integer, SsrcRewriter> rewriters
+ = new ConcurrentHashMap<>();

Same thing here, the `rewriters` object is accessed by a single thread at the time. We should be clear about that tho, to avoid confusion in the future.

···

--
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/libjitsi/pull/196/files/36aed379244c7a28d7d30439da350121c72e3e25#r77413063


#18

@@ -63,11 +63,16 @@ public RawPacket reverseTransform(RawPacket pkt)
                 = (pkt.getFlags() & Buffer.FLAG_DISCARD) == Buffer.FLAG_DISCARD;

             long ssrc = pkt.getSSRCAsLong();
- ResumableStreamRewriter rewriter = ssrcToRewriter.get(ssrc);
- if (rewriter == null)
+
+ ResumableStreamRewriter rewriter;
+ synchronized(ssrcToRewriter)

Same thing here, the `ssrcToRewriter` object is accessed by a single thread at the time. We should be clear about that tho, to avoid confusion in the future.

···

--
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/libjitsi/pull/196/files/36aed379244c7a28d7d30439da350121c72e3e25#r77413332


#19

@@ -168,14 +171,17 @@ public void putResumableStreamRewriter(
     public ResumableStreamRewriter getResumableStreamRewriter(Long ssrc,
                                                               boolean create)
     {
- ResumableStreamRewriter rewriter = ssrcToRewriter.get(ssrc);
- if (rewriter == null && create)
+ synchronized (ssrcToRewriter)

I would rather synchronize the block than depend on the fact that currently it is only used by one thread (the method is public and used in jitsi-videobridge).

···

--
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/libjitsi/pull/196/files/36aed379244c7a28d7d30439da350121c72e3e25#r77413381


#20

@@ -66,7 +68,8 @@
      * A map of SSRCs to <tt>SsrcRewriter</tt>. Each SSRC that we rewrite in
      * this group rewriter has its own rewriter.
      */
- private final Map<Integer, SsrcRewriter> rewriters = new HashMap<>();
+ private final Map<Integer, SsrcRewriter> rewriters
+ = new ConcurrentHashMap<>();

OK, will remove the commit.

···

--
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/libjitsi/pull/196/files/36aed379244c7a28d7d30439da350121c72e3e25#r77414061