[jitsi-dev] [jitsi/libjitsi] Refactors seqnum rewriting for video packets. (#179)


#1

- Extracts the rewriting logic in a re-usable module.
- Reuses the module for rewriting sequence numbers of audio packets.
- Moves all the logic inside the OutputDataStreamImpl (as opposed to
  having half of it in the OutputDataStreamImpl and half of it in
  the VideoChannel).
- Adds tests for the seqnum rewriting.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Refactors seqnum rewriting for video packets.

-- File Changes --

    A src/org/jitsi/impl/neomedia/SequenceNumberRewriter.java (107)
    M src/org/jitsi/impl/neomedia/rtp/translator/OutputDataStreamImpl.java (23)
    M src/org/jitsi/impl/neomedia/transform/csrc/SsrcTransformEngine.java (16)
    M src/org/jitsi/util/RTPUtils.java (6)
    A test/org/jitsi/impl/neomedia/SequenceNumberRewriterTest.java (107)

-- Patch Links --

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


#2

@@ -261,6 +266,17 @@ public RawPacket reverseTransform(RawPacket pkt)
                 csrcAudioLevelDispatcher.addLevels(levels, pkt.getTimestamp());
             }
         }
+
+ SequenceNumberRewriter rewriter = ssrcToRewriter.get(pkt.getSSRCAsLong());

@gpolitis The fact that the rewriter is accessed after the lastN decision means that not all packets will reach here (if write is "false" we never get here) Rewriter needs to see all the packets of all ssrcs.

···

---
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/179/files/e8706451b70dc3fd07df4e9b4a22da875ccb85d6#r71384027


#3

@@ -261,6 +266,17 @@ public RawPacket reverseTransform(RawPacket pkt)
                 csrcAudioLevelDispatcher.addLevels(levels, pkt.getTimestamp());
             }
         }
+
+ SequenceNumberRewriter rewriter = ssrcToRewriter.get(pkt.getSSRCAsLong());

Not sure I understand this. This is in the audio chain, so packets wouldn't have been dropped elsewhere, so all packets will reach the rewriter. Am I missing something?

···

---
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/179/files/e8706451b70dc3fd07df4e9b4a22da875ccb85d6#r71481934


#4

@@ -261,6 +266,17 @@ public RawPacket reverseTransform(RawPacket pkt)
                 csrcAudioLevelDispatcher.addLevels(levels, pkt.getTimestamp());
             }
         }
+
+ SequenceNumberRewriter rewriter = ssrcToRewriter.get(pkt.getSSRCAsLong());

Was the comment meant for the changes to OutputDataStreamImpl.java?

···

---
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/179/files/e8706451b70dc3fd07df4e9b4a22da875ccb85d6#r71483129


#5

@@ -65,6 +65,12 @@
     private final RTPConnectorImpl connector;

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

Don't we need a separate rewriter for each receiver?

···

---
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/179/files/e8706451b70dc3fd07df4e9b4a22da875ccb85d6#r71485324


#6

@@ -261,6 +266,17 @@ public RawPacket reverseTransform(RawPacket pkt)
                 csrcAudioLevelDispatcher.addLevels(levels, pkt.getTimestamp());
             }
         }
+
+ SequenceNumberRewriter rewriter = ssrcToRewriter.get(pkt.getSSRCAsLong());

Thank you both for the comments! I'll update the code accordingly.

···

---
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/179/files/e8706451b70dc3fd07df4e9b4a22da875ccb85d6#r71491098


#7

@@ -65,6 +65,12 @@
     private final RTPConnectorImpl connector;

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

Right, part of the code needs to be in the `OutputStreamImplDesc`.

···

---
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/179/files/e8706451b70dc3fd07df4e9b4a22da875ccb85d6#r71491185


#8

+ RTPUtils.sequenceNumberDiff(newSequenceNumber, highestSent) > 0)
+ {
+ highestSent = newSequenceNumber;
+ }
+
+ return newSequenceNumber;
+ }
+ else
+ {
+ // update the delta (if needed)
+ if (highestSent != -1)
+ {
+ final int d
+ = RTPUtils.subtractNumber(sequenceNumber, highestSent);
+
+ if (delta == 0 ||

Why special handling for delta==0? If delta == 0, d=2^16-1, we want delta to stay 0. Also, can we rename "d"?

···

---
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/179/files/23b9180bb0d772140330495b6a99322e5524124c#r71537999


#9

+ RTPUtils.sequenceNumberDiff(newSequenceNumber, highestSent) > 0)
+ {
+ highestSent = newSequenceNumber;
+ }
+
+ return newSequenceNumber;
+ }
+ else
+ {
+ // update the delta (if needed)
+ if (highestSent != -1)
+ {
+ final int d
+ = RTPUtils.subtractNumber(sequenceNumber, highestSent);
+
+ if (delta == 0 ||

Right :+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/libjitsi/pull/179/files/23b9180bb0d772140330495b6a99322e5524124c#r71539235


#10

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/179#issuecomment-234058493


#11

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/179#issuecomment-234089984


#12

@@ -261,6 +267,17 @@ public RawPacket reverseTransform(RawPacket pkt)
                 csrcAudioLevelDispatcher.addLevels(levels, pkt.getTimestamp());
             }
         }
+
+ SequenceNumberRewriter rewriter = ssrcToRewriter.get(pkt.getSSRCAsLong());
+ if (rewriter == null)
+ {
+ rewriter = new SequenceNumberRewriter();
+ ssrcToRewriter.put(pkt.getSSRCAsLong(), rewriter);
+ }
+
+ rewriter.rewrite(
+ !dropPkt, pkt.getBuffer(), pkt.getOffset(), pkt.getLength());

This transformer is placed in the chain before SRTP. If it rewrites sequence numbers SRTP authentication will fail :frowning:

···


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/179/files/abfcb306ddce2279fb738fbae9df5a48d4e5e775..32f9749a1565731b8eb0f48c486bbab6c81b8135#r71653904


#13

                         /* destination */ streamRTPManager,
- _data);
+ _data);
+ }
+
+ // Hide gaps in the sequence numbers because of dropping packets.
+ Long ssrc = RawPacket.getSSRCAsLong(buf, off, len);

This code executes for RTCP as well. I suspect that up until now it somehow worked without major issues, but with this it will quickly fill the map since getSSRCAsLong will actually read the NTP timestamp if the packet is RTCP, and that changes with each packet.

···


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/179/files/2531b2d521bc5634a094249f8108b65305070711..abfcb306ddce2279fb738fbae9df5a48d4e5e775#r71655010


#14

@gpolitis pushed 2 commits.

f385ecb Moves a member field to a more appropriate place.
c5bbe3b Rewrite sequence numbers only for RTP packets.

···


You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jitsi/libjitsi/pull/179/files/32f9749a1565731b8eb0f48c486bbab6c81b8135..c5bbe3b123e6a806cc75dd484ae8077313692607


#15

@gpolitis pushed 2 commits.

bceea1a Revert "Reuses the module for rewriting sequence numbers of audio packets."
f0562e5 Rewrites sequence numbers for dropped audio packets.

···


You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jitsi/libjitsi/pull/179/files/c5bbe3b123e6a806cc75dd484ae8077313692607..f0562e55819e754fb4fadb6977f6f147515c69d3


#16

@bgrozev pushed 4 commits.

84f4e9f Moves the RTP sequence number rewriter to the .rtp package.
66006fc Clarifies comments. Only tries to reset the sequence number in case of
f53f6dd Only uses a DiscardTransformEngine for audio.
9b98764 Moves DiscardTransformEngine to the transform package.

···


You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jitsi/libjitsi/pull/179/files/f0562e55819e754fb4fadb6977f6f147515c69d3..9b9876402bad6393b0b91f782a3233d95574c116


#17

I think this is good to go now. @gpolitis can you please check if my changes make sense?

@brianh5 @mdaneshi Would you be able to test this in your scenario?

···

---
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/179#issuecomment-234462778


#18

@gpolitis pushed 1 commit.

311053d Restores the original seqnum only if it was changed.

···


You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jitsi/libjitsi/pull/179/files/9b9876402bad6393b0b91f782a3233d95574c116..311053dc5c98aedd3ab0cbcca31d42ee4938b651


#19

LGTM

···

---
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/179#issuecomment-234515790


#20

Merging as this has been manually tested, the automated tests have passed and we need this deployed.

···

---
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/179#issuecomment-235053662