[jitsi-dev] [jitsi/libjitsi] Rewriting refactoring (#201)


#1

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

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

-- Commit Summary --

  * refactor(concurrency): RecurringProcessible -> RecurringRunnable .
  * ref(rtcp): Removes dead code.
  * ref(rtcp): Makes the CNAMERegistry an inner class.
  * ref(rtcp): Wrap anonymous classes into inner classes.
  * ref(stream): Moves the worker thread inside the MediaStreamImpl.
  * ref(rtcp): Makes the rtcp termination strategy a recurring runnable.
  * ref(rewriting): Sanitizes SSRC rewriting.

-- File Changes --

    M src/org/jitsi/impl/neomedia/MediaStreamImpl.java (129)
    M src/org/jitsi/impl/neomedia/VideoMediaStreamImpl.java (48)
    M src/org/jitsi/impl/neomedia/rtcp/termination/strategies/BasicRTCPTerminationStrategy.java (223)
    D src/org/jitsi/impl/neomedia/rtcp/termination/strategies/CNAMERegistry.java (70)
    D src/org/jitsi/impl/neomedia/rtcp/termination/strategies/FeedbackCache.java (80)
    D src/org/jitsi/impl/neomedia/rtcp/termination/strategies/FeedbackCacheEntry.java (30)
    D src/org/jitsi/impl/neomedia/rtcp/termination/strategies/FeedbackCacheProcessor.java (339)
    M src/org/jitsi/impl/neomedia/rtp/remotebitrateestimator/RemoteBitrateEstimatorSingleStream.java (5)
    M src/org/jitsi/impl/neomedia/rtp/sendsidebandwidthestimation/BandwidthEstimatorImpl.java (7)
    M src/org/jitsi/impl/neomedia/rtp/translator/RTCPFeedbackMessageSender.java (26)
    M src/org/jitsi/impl/neomedia/stats/MediaStreamStats2Impl.java (9)
    M src/org/jitsi/impl/neomedia/stats/SendTrackStatsImpl.java (9)
    M src/org/jitsi/impl/neomedia/transform/CachingTransformer.java (23)
    M src/org/jitsi/impl/neomedia/transform/rewriting/ExtendedSequenceNumberInterval.java (391)
    D src/org/jitsi/impl/neomedia/transform/rewriting/SsrcGroupRewriter.java (569)
    D src/org/jitsi/impl/neomedia/transform/rewriting/SsrcRewriter.java (641)
    M src/org/jitsi/impl/neomedia/transform/rewriting/SsrcRewritingEngine.java (1106)
    M src/org/jitsi/service/neomedia/AbstractMediaStream.java (17)
    M src/org/jitsi/service/neomedia/MediaStream.java (63)
    A src/org/jitsi/service/neomedia/MediaStreamTrack.java (97)
    A src/org/jitsi/service/neomedia/RTPEncoding.java (117)
    M src/org/jitsi/service/neomedia/stats/SendTrackStats.java (5)
    D src/org/jitsi/util/RefCount.java (56)
    R src/org/jitsi/util/concurrent/PeriodicRunnable.java (32)
    R src/org/jitsi/util/concurrent/PeriodicRunnableWithObject.java (32)
    R src/org/jitsi/util/concurrent/RecurringRunnable.java (15)
    R src/org/jitsi/util/concurrent/RecurringRunnablesExecutor.java (129)

-- Patch Links --

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


#2

lyubomir requested changes on this pull request.

  */

-public class RecurringProcessibleExecutor
+public class RecurringRunnablesExecutor

I don't consider the use of the plural case a common practice. Like `RecurringProcessibleExecutor`, I find the single-case name `RecurringRunnableExecutor` better than `RecurringRunnablesExecutor`.

···

      * worker thread as Process will be called on.

      *
      * @return the number of milliseconds until this instance wants a worker
- * thread to call {@link #process()}
+ * thread to call {@link #run()}
      */
     long getTimeUntilNextProcess();

I take it the goal of the renaming from `Processible` to `Runnable` was to simplify the interface, especially the method `process` with `its ignored (and unknown) at the time of this writing` return value. However, I believe you should've gone the extra step of renaming the method `getTimeUntilNextProcess` to `getTimeUntilNextRun`. As it stands right now, I don't think the method name is as clear as possible.

@@ -149,22 +150,22 @@ else if (command instanceof RecurringProcessible)

      */
     private boolean process()

If we're to get rid of `processible` in the names, I don't understand why you're leaving `process`.

@@ -77,6 +78,14 @@

         = Logger.getLogger(MediaStreamImpl.class);

     /**
+ * The <tt>RecurringRunnablesExecutor</tt> to be utilized by the
+ * <tt>MediaStreamImpl</tt> class and its instances.
+ */
+ protected static final RecurringRunnablesExecutor
+ recurringRunnablesExecutor
+ = new RecurringRunnablesExecutor(MediaStreamImpl.class.getSimpleName());

I don't agree with such source code formatting/indentation.

@@ -1029,7 +1054,9 @@ private TransformEngineChain createTransformEngineChain()

         if (redTransformEngine != null)
             engineChain.add(redTransformEngine);

- engineChain.add(ssrcRewritingEngine);
+ SsrcRewritingEngine ssrcRewritingEngine = getSsrcRewritingEngine();
+ if (ssrcRewritingEngine != null)
+ engineChain.add(ssrcRewritingEngine);

It seems that we've tried to put comments at top of such groups before giving a brief "label" to the group. Wouldn't it be nice to be consistent and put `// SSRC rewriting` at the top of the group here as well?

@@ -771,6 +778,14 @@ public void close()

         if (deviceSession != null)
             deviceSession.close();
+
+ RTCPTerminationStrategy strategy
+ = rtcpTransformEngineWrapper.getWrapped();

There's a `MediaStreamImpl#getRTCPTerminationStrategy()` method which amounts to `rtcpTransformEngineWrapper.getWrapped()`. Why not use it here and in the method `setRTCPTerminationStrategy` and, thus, simplify the source code for the reader?

@@ -771,6 +778,14 @@ public void close()

         if (deviceSession != null)
             deviceSession.close();
+
+ RTCPTerminationStrategy strategy
+ = rtcpTransformEngineWrapper.getWrapped();
+ if (strategy instanceof RecurringRunnable)
+ {
+ recurringRunnablesExecutor
+ .deRegisterRecurringRunnable((RecurringRunnable) strategy);
+ }

It disturbs me that we're continually adding to `MediaStreamImpl` functionality that can go outside of it without substantial designing and/or refactoring. We've added `cashingTransformer.close()`, `retransmissionRequester.close()` and now rtcpTransformEngineWrapper given that we call `PacketTransformer#close()` on all `TransformEngine`s in `transformEngineChain`. I don't see why these are being done here given that any `TransformEngine` is closed (through its `PacketTransformer`s).

     {

- long ret;
-
         try
         {
             doProcess();

Again, `processible` gets renamed to `runnable`, `process` to `run`, yet `doProcess` remains as it is. I think that's an easily avoidable inconsistency.

@@ -91,10 +91,8 @@ public long getTimeUntilNextProcess()

      * Updates {@link #_lastProcessTime}.
      */
     @Override
- public long process()
+ public void run()
     {
         _lastProcessTime = System.currentTimeMillis();

Again, `processible` gets renamed to `runnable`, `process` to `run`, yet `_lastProcessTime ` remains as it is. I think that's an easily avoidable inconsistency.

+ /**

+ * Gets the remote {@code MediaStreamTrack} that matches the given SSRC.
+ *
+ * @param ssrc the SSRC to match the {@code MediaStreamTrack} against. It
+ * can be the primary SSRC or the RTX SSRC.
+ *
+ * @return the remote {@code MediaStreamTrack} that matches the given SSRC,
+ * or null if there's no matching {@code MediaStreamTrack}.
+ */
+ public MediaStreamTrack getRemoteTrack(long ssrc);
+
+ /**
+ * Clears the remote {@code MediaStreamTrack}s associated with this
+ * {@code MediaStream}.
+ */
+ public void clearRemoteTracks();

From what I can see from the source code, you pretty much want the collection of `MediaStreamTrack`s to be associated with `MediaStream`. Probably, because you have a `MediaStream` somewhere and want to be able to get the associated `MediaStreamTrack`s. But `MediaStreamImpl` itself doesn't even seem to be owning that collection because additions and removals (i.e. `clearRemoteTracks()`) are being done from the outside. So to me it's an appendage that's polluting already complicated interface and implementation. I think it would've been simple to add a `MediaStreamTrack(Collection|List|Set)` abstraction that has all these `MediaStreamTrack`-related methods and then `MediaStream(Impl)` would've had a single new accessor.

         {

- if (!timeToSendRTCPReport())
- {
- return;
- }
+ long now = System.currentTimeMillis();
+ lastUpdateTimeMs = now;

What's the point of the now variable here? Doesn't it just turn 1 line into 2?

+ /**

+ * {@inheritDoc}
+ */
+ @Override
+ public RawPacket transform(RawPacket pkt)
+ {
+ RTCPCompoundPacket compound;
+
+ try
+ {
+ compound
+ = (RTCPCompoundPacket)
+ parser.parse(
+ pkt.getBuffer(),
+ pkt.getOffset(),
+ pkt.getLength());

I don't approve of such source code styling/indentation.

@@ -1829,6 +1874,57 @@ public long getRemoteSourceID()

     }

     /**
+ * @{inheritDoc}
+ */
+ @Override
+ public void addRemoteTrack(MediaStreamTrack mediaStreamTrack)
+ {
+ if (mediaStreamTrack == null)
+ {
+ return;
+ }
+
+ Set<Long> ssrcs = mediaStreamTrack.getEncodingsBySSRC().keySet();
+
+ if (ssrcs == null || ssrcs.size() == 0)

1. Standard `Map` implementation do not return `null` from `keySet()` so I find it an unnecessary complication of the source code to check for `null`. If anyone is implementing a `keySet` method that returns `null`, I consider it their own problem.

2. `ssrcs.size() == 0` is more clearly expressed as `ssrcs.isEmpty()` and this fact is reported by (certain) static analysis tools.

@@ -1019,7 +984,7 @@ public void cleanup()

             // TAG(cat4-remote-ssrc-hurricane) first. The idea is to remove
             // from our data structures everything that is not listed in as
             // a remote SSRC.
- }
+ }

I don't get the above coding style/indentation modification...

+ RTCPSDESItem[] items = chunk.items;

+
+ if (items == null || items.length == 0)
+ continue;
+
+ for (RTCPSDESItem item : items)
+ {
+ if (RTCPSDESItem.CNAME == item.type)
+ put(chunk.ssrc, item.data);
+ }
+ }
+ }
+ }
+ }
+
+ class RTCPPacketTransformerImpl

1. I don't know that extracting RTPPacketTransformerImpl and RTCPPacketTransformerImpl made the source code better. Sure, indentation was removed which is an advantage. A disadvantage is that you had to add constructors.

2. Why are these 2 classes not private?

+ RTPEncoding encoding = new RTPEncoding(this, primarySSRC, rtxSSRC, fecSSRC);

+
+ if (encoding.getPrimarySSRC() != -1)
+ {
+ encodingsBySSRC.put(encoding.getPrimarySSRC(), encoding);
+ }
+
+ if (encoding.getRTXSSRC() != -1)
+ {
+ encodingsBySSRC.put(encoding.getRTXSSRC(), encoding);
+ }
+
+ if (encoding.getFECSSRC() != -1)
+ {
+ encodingsBySSRC.put(encoding.getFECSSRC(), encoding);
+ }

I don't like the above 3 `if`s. I believe a simple function that captures the common source code will be easier to understand.

--
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/201#pullrequestreview-1738312


#3

Thanks very much for your valuable feedback @lyubomir, I really appreciate it. I will take care of everything that you mention tomorrow.

···

--
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/201#issuecomment-250056025


#4

gpolitis commented on this pull request.

@@ -771,6 +778,14 @@ public void close()

         if (deviceSession != null)
             deviceSession.close();

···

+
+ RTCPTerminationStrategy strategy
+ = rtcpTransformEngineWrapper.getWrapped();
+ if (strategy instanceof RecurringRunnable)
+ {
+ recurringRunnablesExecutor
+ .deRegisterRecurringRunnable((RecurringRunnable) strategy);
+ }

I agree this is disturbing. To quote @bgrozev from the commit message:

Closes #cachingTransformer on MediaStreamImpl#close(), fixing a leak when jitsi-videobridge performs health checks. We cannot rely on it being closed with #transformEngineChain, because the chain isn't always initialized (when helath-checks are run on jitsi-videobridge, it creates MediaStreams but never sets their RTPConnector, so the chain is never created.

Can we leave it as is for now, or you think this is completely unacceptable?

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


#5

@gpolitis pushed 7 commits.

e6662eb ref(concurrency): Renames RecurringRunnablesExecutor -> RecurringRunnableExecutor.
9039c8b style(media): Fixes indentation.
aa00ad2 style(media): Code reuse.
5c63b13 ref(concurrency): Renames *process to *run.
443408f docs(media): Labels the SSRC rewriting engine.
dc7fa36 ref(media): Simplifies code.
f1077f8 ref(rtcp): Removes dead code.

···


You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jitsi/libjitsi/pull/201/files/c92662517dd9d725dc72e5b883a69d16999c09ed..f1077f8786c26e6d3116a3f411e2a61f7e817ca7


#6

gpolitis commented on this pull request.

  */

-public class RecurringProcessibleExecutor
+public class RecurringRunnablesExecutor

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/libjitsi/pull/201


#7

gpolitis commented on this pull request.

@@ -149,22 +150,22 @@ else if (command instanceof RecurringProcessible)

      */
     private boolean process()

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/libjitsi/pull/201


#8

gpolitis commented on this pull request.

···

      * worker thread as Process will be called on.

      *
      * @return the number of milliseconds until this instance wants a worker
- * thread to call {@link #process()}
+ * thread to call {@link #run()}
      */
     long getTimeUntilNextProcess();

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/libjitsi/pull/201


#9

gpolitis commented on this pull request.

@@ -77,6 +78,14 @@

         = Logger.getLogger(MediaStreamImpl.class);

     /**
+ * The <tt>RecurringRunnablesExecutor</tt> to be utilized by the
+ * <tt>MediaStreamImpl</tt> class and its instances.
+ */
+ protected static final RecurringRunnablesExecutor
+ recurringRunnablesExecutor
+ = new RecurringRunnablesExecutor(MediaStreamImpl.class.getSimpleName());

Does it look better now?

···

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


#10

gpolitis commented on this pull request.

@@ -771,6 +778,14 @@ public void close()

         if (deviceSession != null)
             deviceSession.close();

···

+
+ RTCPTerminationStrategy strategy
+ = rtcpTransformEngineWrapper.getWrapped();

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/libjitsi/pull/201


#11

gpolitis commented on this pull request.

@@ -1029,7 +1054,9 @@ private TransformEngineChain createTransformEngineChain()

         if (redTransformEngine != null)
             engineChain.add(redTransformEngine);

- engineChain.add(ssrcRewritingEngine);
+ SsrcRewritingEngine ssrcRewritingEngine = getSsrcRewritingEngine();
+ if (ssrcRewritingEngine != null)
+ engineChain.add(ssrcRewritingEngine);

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/libjitsi/pull/201


#12

gpolitis commented on this pull request.

@@ -1829,6 +1874,57 @@ public long getRemoteSourceID()

     }

     /**
+ * @{inheritDoc}
+ */
+ @Override
+ public void addRemoteTrack(MediaStreamTrack mediaStreamTrack)
+ {
+ if (mediaStreamTrack == null)
+ {
+ return;
+ }

···

+
+ Set<Long> ssrcs = mediaStreamTrack.getEncodingsBySSRC().keySet();
+
+ if (ssrcs == null || ssrcs.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/libjitsi/pull/201


#13

gpolitis commented on this pull request.

     {

- long ret;

···

-
         try
         {
             doProcess();

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/libjitsi/pull/201


#14

gpolitis commented on this pull request.

@@ -91,10 +91,8 @@ public long getTimeUntilNextProcess()

      * Updates {@link #_lastProcessTime}.
      */
     @Override
- public long process()
+ public void run()
     {
         _lastProcessTime = System.currentTimeMillis();

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/libjitsi/pull/201


#15

gpolitis commented on this pull request.

@@ -1019,7 +984,7 @@ public void cleanup()

             // TAG(cat4-remote-ssrc-hurricane) first. The idea is to remove
             // from our data structures everything that is not listed in as
             // a remote SSRC.
- }
+ }

Me neither :slight_smile: 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/libjitsi/pull/201


#16

gpolitis commented on this pull request.

         {

- if (!timeToSendRTCPReport())
- {
- return;
- }
+ long now = System.currentTimeMillis();
+ lastUpdateTimeMs = now;

It has no point whatsoever. 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/libjitsi/pull/201


#17

gpolitis commented on this pull request.

+ /**

+ * {@inheritDoc}
+ */
+ @Override
+ public RawPacket transform(RawPacket pkt)
+ {
+ RTCPCompoundPacket compound;

···

+
+ try
+ {
+ compound
+ = (RTCPCompoundPacket)
+ parser.parse(
+ pkt.getBuffer(),
+ pkt.getOffset(),
+ pkt.getLength());

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/libjitsi/pull/201


#18

@gpolitis pushed 1 commit.

f0e05e4 style(rtcp): Makes inner classes private.

···


You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jitsi/libjitsi/pull/201/files/f1077f8786c26e6d3116a3f411e2a61f7e817ca7..f0e05e464f83c1d506c38bc164208e86f96b418c


#19

gpolitis commented on this pull request.

···

+ RTCPSDESItem[] items = chunk.items;

+
+ if (items == null || items.length == 0)
+ continue;
+
+ for (RTCPSDESItem item : items)
+ {
+ if (RTCPSDESItem.CNAME == item.type)
+ put(chunk.ssrc, item.data);
+ }
+ }
+ }
+ }
+ }
+
+ class RTCPPacketTransformerImpl

I don't have a very good answer to that; I personally prefer seeing the implementation after the constructor. This is subjective and I can revert the commit, if you'd like me to. In the meantime, I made the two classes private.

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


#20

@gpolitis pushed 1 commit.

619ddd2 style(rtcp): Makes inner classes private.

···


You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jitsi/libjitsi/pull/201/files/f0e05e464f83c1d506c38bc164208e86f96b418c..619ddd201eb7fbf2580f2d8bf2d660a14ee64657