[jitsi-dev] [jitsi/libjitsi] RTP uplifting frame reordering fixes (#193)


#1

Sure, so I was analyzing a pcap file and I noticed re-order frames which the bridge mis-handles and causes it to break the RTP stream.

Say you send frames A B C and each is comprised of 3 RTP packets (3 is not important, it can be N). So the sender emits A1, A2, A3, B1, B2, B3, C1, C2, C3. If the bridge receives something like this A1, A2, A3, C1, B1, B2, B3, C2, C3 it will break the RTP stream because it will uplift the B frame to Timestamp_C+1.

tl;dr; the RTP uplifting logic can cope with some packet re-ordering, but it doesn’t expect whole frames to be re-ordered. I’ve confirmed that it can cause long freezes; I’m not sure about scrambling but I find it much less likely.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * refactor: Limits the RTP timestamp history to 20 entries.
  * refactor: Checks for timestamp freshness before using it.
  * refactor: Takes care of frame re-ordering.

-- File Changes --

    M src/org/jitsi/impl/neomedia/transform/rewriting/SsrcRewriter.java (83)

-- Patch Links --

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


#2

@gpolitis pushed 1 commit.

df481bd refactor: Minimizes System.currentTimeMillis usage.

···


You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jitsi/libjitsi/pull/193/files/04859cc470bdfbeb1cc8cfe786a2575a3c734935..df481bde442f5b2e7b398bdbf3bf14a0aefbb843


#3

@gpolitis pushed 1 commit.

4092810 refactor: Moves the RemoteClockEstimator.

···


You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jitsi/libjitsi/pull/193/files/81ac0cb1389305cd52e511f51dd9b4033e5d6203..40928103ced07538f11a2c3d238e7af6b26c24d8


#4

@gpolitis pushed 1 commit.

07f8bf1 refactor: Uses long for SSRCs.

···


You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jitsi/libjitsi/pull/193/files/40928103ced07538f11a2c3d238e7af6b26c24d8..07f8bf18b73ca43d4d7e9e16c791ed8632922a66


#5

I might have missed some discussion, but the change int -> long in the SSRCs changes a lot of public API. I understand the motivation (and hate once again Java for not having uints), but is this really necessary? At some point much care was taken to NOT use longs and things refactored from long to int...

···

--
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/193#issuecomment-242137663


#6

@ibauersachs thanks for chiming in. I'm afraid of regressions so there's a good chance I'll take that back. I'm not aware of any decision to use ints instead of longs (fmj favors ints for sure) and it seems we're in favor of longs in LJ (at least in the past couple of years). I have reviewed a lot of PRs from @bgrozev and he's using them whenever possible. cc @lyubomir

···

--
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/193#issuecomment-242139943


#7

@gpolitis pushed 1 commit.

6971c1f refactor: fixes buffer access

···


You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jitsi/libjitsi/pull/193/files/0d91f4d6d90977d2f2c4133560e1b894914ce456..6971c1f51e9eca7c7e7f7d7e000c55e258349491


#8

@gpolitis pushed 1 commit.

c1efd91 logs: Prints real time next to RTP timestamps.

···


You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jitsi/libjitsi/pull/193/files/718bba11207f74ee22dd72b683cc418edc5f475b..c1efd91a2081dec0e2960f85a49b5d22d5c09660


#9

@gpolitis pushed 1 commit.

e21c6e4 logs: The remote clock estimator logger now includes the stream hashCode.

···


You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jitsi/libjitsi/pull/193/files/8b6c9b8d9a9d48e22ee7359f50f11ae52ea2db6e..e21c6e4b233aab710987c3a0598ed479cb4396d3


#10

@gpolitis pushed 8 commits.

3decb91 logs: mediastream hashcode
63db24d logs: parsable
255418c Fixes the remote clock frequency calculation.
4088fcf Mandaroty update of the remote clock when an SR is received.
592d886 logs logs logs
81cb95a logs logs logs
b48d47a logs logs logs
f2c5b9c Makes it possible to alter the state of the ResumableStreamRewriter from both RTP/RTCP.

···


You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jitsi/libjitsi/pull/193/files/e21c6e4b233aab710987c3a0598ed479cb4396d3..f2c5b9cd573432d29d8dcc3f4242b45611f199e6


#11

@gpolitis pushed 1 commit.

12b91d0 [rewriting] Prevents an NPE when the frequency is less than 1 kHz.

···


You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jitsi/libjitsi/pull/193/files/95437689e07b93922f5e6aec7c23941b0f988db2..12b91d0ad682f114b71daa1a1c286f092de8d748


#12

jenkins it's ok to test

···

--
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/193#issuecomment-243877422


#13

@gpolitis pushed 1 commit.

25b204b [rewriting] Dummy method implementation in AbstractMediaStreamImpl.

···


You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jitsi/libjitsi/pull/193/files/6d0991f38becb6d857e1df94ee6aef2e31c945f8..25b204b47c09c50ff6eedd2b48d15d1505e42fd3


#14

@@ -106,6 +105,11 @@ protected boolean removeEldestEntry(Map.Entry eldest) {
         }
     };

+ /*
+ *
+ */
+ private TimestampEntry maxSourceTsEntry;

Missing javadoc :slight_smile:

···


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/193/files/0a3edb08537a96edd3571f636e7bd41eda4051ba..7617c9988acdb18cc7943756f0747dd174d3dbf1#r77077035


#15

@@ -287,7 +364,8 @@ void rewriteTimestamp(RawPacket p)
                     + " to " + newValue);
             }

- tsHistory.put(oldValue, new TimestampEntry(now, newValue));
+ tsHistory.put(oldValue, new TimestampEntry(now, oldValue, newValue));
+ maxSourceTsEntry = new TimestampEntry(now, oldValue, newValue);
         }

Duplicate object creation.

···


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/193/files/0a3edb08537a96edd3571f636e7bd41eda4051ba..7617c9988acdb18cc7943756f0747dd174d3dbf1#r77077144


#16

      * A map of source ssrc to {@link ResumableStreamRewriter}.
      */
     private final Map<Long, ResumableStreamRewriter> ssrcToRewriter
         = new HashMap<>();

     /**
+ *
+ */
+ private final MediaStream stream;
+

Missing javadoc.

···


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/193/files/6439e938a0d770a673cec03a7aebccfa06360408..e54ebb228a59e1bbc94b8e4ac5738e7490643209#r77078472


#17

      * A map of source ssrc to {@link ResumableStreamRewriter}.
      */
     private final Map<Long, ResumableStreamRewriter> ssrcToRewriter
         = new HashMap<>();

     /**
+ *
+ */
+ private final MediaStream stream;
+
+ /**
+ *
+ * @param stream
+ */
+ public DiscardTransformEngine(MediaStream stream)
+ {

Missing javadoc.

···


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/193/files/6439e938a0d770a673cec03a7aebccfa06360408..e54ebb228a59e1bbc94b8e4ac5738e7490643209#r77078509


#18

@@ -128,11 +140,15 @@ public void update(byte[] buf, int off, int len)
             }
         }

+
+ logger.debug("Updating the remote clock ssrc=" + ssrc
+ + ", systemTimeMs=" + systemTimeMs
+ + ", rtpTimestamp=" + rtptimestamp);
+

Maybe add if (logger.isDebugEnabled()) to avoid string creation.

···


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/193/files/da8c750ed9e3da256cb9f4022bbc03bba1a4b6f6..2796706394c64b8376582d90f3c4ca8da4ee6cbf#r77080350


#19

@@ -52,6 +52,8 @@
      */
     long highestTimestampSent = -1;

+ private int cycles = 0;
+

javadoc missing

···


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/193/files/a97f8e0f11a502e1fe8bfc88d639cb744dd2a3f8..87f71421e59531e8a2382fcb398d3688227b0f65#r77083029


#20

@gpolitis pushed 2 commits.

c5f22b2 [rewriting] Avoids duplicate object creation.
43ec2e0 [rewriting] Adds missing JavaDoc.

···


You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jitsi/libjitsi/pull/193/files/25b204b47c09c50ff6eedd2b48d15d1505e42fd3..43ec2e0e74a537ccc476a5869542d620f38a203a