[jitsi-dev] [jitsi/libjitsi] RTP stream pause/resume improvements. (#181)


#1

Rewrites timestamps in addition to sequence numbers when a stream is paused/resumed.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Rewrites timestamps when a stream is paused/resumed.
  * SequenceNumberRewriter -> ResumableStreamRewriter.

-- File Changes --

    M src/org/jitsi/impl/neomedia/RawPacket.java (38)
    A src/org/jitsi/impl/neomedia/rtp/ResumableStreamRewriter.java (203)
    D src/org/jitsi/impl/neomedia/rtp/SequenceNumberRewriter.java (112)
    M src/org/jitsi/impl/neomedia/rtp/StreamRTPManager.java (4)
    M src/org/jitsi/impl/neomedia/rtp/translator/OutputDataStreamImpl.java (9)
    M src/org/jitsi/impl/neomedia/transform/DiscardTransformEngine.java (9)
    M src/org/jitsi/util/RTPUtils.java (16)
    R test/org/jitsi/impl/neomedia/rtp/ResumableStreamRewriterTest.java (60)

-- Patch Links --

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


#2

is this in response to https://bugs.chromium.org/p/webrtc/issues/detail?id=5868&can=1&q=is%3Astarred%20&colspec=ID%20Pri%20Mstone%20ReleaseBlock%20Component%20Status%20Owner%20Summary ? or simulcast streams stopping/starting. (or i suppose both).

···

---
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/181#issuecomment-235674635


#3

@brianh5 neither, it's much simpler than that :slight_smile: When hiding the sequence number gaps (because of last N, for example), we also need to hide the gaps in the timestamps. Otherwise the jitter buffer might freak out. Does that make sense?

···

---
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/181#issuecomment-235675835


#4

gotcha.

···

---
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/181#issuecomment-235676716


#5

@gpolitis pushed 3 commits.

e56c988 Adds logs
0873068 Fixes JavaDoc.
061ccaa Rewrites RTP timestamps in SRs.

···


You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jitsi/libjitsi/pull/181/files/a408aebdc7f6b3cb6cbe4cc72d391a8a0f170a74..061ccaa92c90fab568d16cdb9360b419aa201457


#6

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/181#issuecomment-236658536


#7

@gpolitis pushed 1 commit.

63bc622 Adds a FIXME comment.

···


You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jitsi/libjitsi/pull/181/files/061ccaa92c90fab568d16cdb9360b419aa201457..63bc6229253ad243d7f8c3bef3345545f727b706


#8

         = new HashMap<>();

     /**
+ *

Non-blocking: can we add documentation and move these after the constructor?

···


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/181/files/087306834c35d5ac0153ebc51f49037c5d6d38ef..061ccaa92c90fab568d16cdb9360b419aa201457#r74081138


#9

+
+ return rewrite
+ ? rewriter.rewriteRTP(write, buf, off, len)
+ : rewriter.restoreRTP(buf, off, len);
+ }
+ else
+ {
+ int offset = off, length = len;
+
+ boolean modified = false;
+
+ while (length > 0)
+ {
+ // Check RTCP packet validity. This makes sure that pktLen > 0
+ // so this loop will eventually terminate.
+ if (RTCPHeaderUtils.isValid(buf, offset, length))

Shouldn't this break if the packet is not valid?

···


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/181/files/087306834c35d5ac0153ebc51f49037c5d6d38ef..061ccaa92c90fab568d16cdb9360b419aa201457#r74084507


#10

+ */
+ public boolean processRTCP(boolean rewrite, byte[] buf, int off, int len)
+ {
+ if (timestampDelta == 0)
+ {
+ return false;
+ }
+
+ long ts = RTCPSenderInfoUtils.getTimestamp(buf, off, len);
+ if (ts == -1)
+ {
+ return false;
+ }
+
+ long newTs = rewrite
+ ? (ts - timestampDelta) & 0xffffffff

The constant should be a long (0xffffffffL)

···


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/181/files/087306834c35d5ac0153ebc51f49037c5d6d38ef..061ccaa92c90fab568d16cdb9360b419aa201457#r74086657


#11

+ * @author George Politis
+ */
+public class RTCPHeaderUtils
+{
+ /**
+ * Gets the RTCP packet type.
+ *
+ * @param buf the byte buffer that contains the RTCP header.
+ * @param off the offset in the byte buffer where the RTCP header starts.
+ * @param len the number of bytes in buffer which constitute the actual
+ * data.
+ * @return the unsigned RTCP packet type, or -1 in case of an error.
+ */
+ public static int getPacketType(byte[] buf, int off, int len)
+ {
+ if (buf == null || Math.min(buf.length, len) < off + 2)

off needs to be included in the check for buf.length, but not for len. E.g. the following is OK:
buf.length=10000, off=5000, len=10

···


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/181/files/087306834c35d5ac0153ebc51f49037c5d6d38ef..061ccaa92c90fab568d16cdb9360b419aa201457#r74088321


#12

+
+ return buf[off + 1] & 0xff;
+ }
+
+ /**
+ * Gets the RTCP sender SSRC.
+ *
+ * @param buf the byte buffer that contains the RTCP header.
+ * @param off the offset in the byte buffer where the RTCP header starts.
+ * @param len the number of bytes in buffer which constitute the actual
+ * data.
+ * @return the unsigned RTCP sender SSRC, or -1 in case of an error.
+ */
+ public static long getSenderSSRC(byte[] buf, int off, int len)
+ {
+ if (buf == null || Math.min(buf.length, len) < off + 8)

Same as above

···


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/181/files/087306834c35d5ac0153ebc51f49037c5d6d38ef..061ccaa92c90fab568d16cdb9360b419aa201457#r74088358


#13

+ | ((buf[off + 6] & 0xff) << 8)
+ | (buf[off + 7] & 0xff)) & 0xffffffffl;
+ }
+
+ /**
+ * Gets the RTCP packet length in bytes.
+ *
+ * @param buf the byte buffer that contains the RTCP header.
+ * @param off the offset in the byte buffer where the RTCP header starts.
+ * @param len the number of bytes in buffer which constitute the actual
+ * data.
+ * @return the RTCP packet length in bytes, or -1 in case of an error.
+ */
+ public static int getLength(byte[] buf, int off, int len)
+ {
+ if (buf == null || Math.min(buf.length, len) < off + 4)

Same as above

···


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/181/files/087306834c35d5ac0153ebc51f49037c5d6d38ef..061ccaa92c90fab568d16cdb9360b419aa201457#r74088429


#14

+
+ return (lengthInWords + 1) * 4;
+ }
+
+ /**
+ * Gets the RTCP packet version.
+ *
+ * @param buf the byte buffer that contains the RTCP header.
+ * @param off the offset in the byte buffer where the RTCP header starts.
+ * @param len the number of bytes in buffer which constitute the actual
+ * data.
+ * @return the RTCP packet version, or -1 in case of an error.
+ */
+ public static int getVersion(byte[] buf, int off, int len)
+ {
+ if (buf == null || Math.min(buf.length, len) < off + 1)

Same as above

···


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/181/files/087306834c35d5ac0153ebc51f49037c5d6d38ef..061ccaa92c90fab568d16cdb9360b419aa201457#r74088551


#15

+ * @param buf the byte buffer that contains the RTCP header.
+ * @param off the offset in the byte buffer where the RTCP header starts.
+ * @param len the number of bytes in buffer which constitute the actual
+ * data.
+ * @return true if the RTCP packet is valid, false otherwise.
+ */
+ public static boolean isValid(byte[] buf, int off, int len)
+ {
+ int version = RTCPHeaderUtils.getVersion(buf, off, len);
+ if (version != RTCPHeader.VERSION)
+ {
+ return false;
+ }
+
+ int pktLen = RTCPHeaderUtils.getLength(buf, off, len);
+ if (pktLen < RTCPHeader.SIZE)

Should probably take the upper bound into account as well (len and buf.length)

···


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/181/files/087306834c35d5ac0153ebc51f49037c5d6d38ef..061ccaa92c90fab568d16cdb9360b419aa201457#r74088720


#16

+ */
+public class RTCPSenderInfoUtils
+{
+ /**
+ * Gets the RTP timestamp.
+ *
+ * @param buf the byte buffer that contains the RTCP header.
+ * @param off the offset in the byte buffer where the RTCP sender info
+ * starts.
+ * @param len the number of bytes in buffer which constitute the actual
+ * data.
+ * @return the RTP timestamp, or -1 in case of an error.
+ */
+ public static long getTimestamp(byte[] buf, int off, int len)
+ {
+ if (buf == null || Math.min(buf.length, len) < off + 12)

Same as above

···


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/181/files/087306834c35d5ac0153ebc51f49037c5d6d38ef..061ccaa92c90fab568d16cdb9360b419aa201457#r74088905


#17

+ *
+ * @param buf the byte buffer that contains the RTCP header.
+ * @param off the offset in the byte buffer where the RTCP sender info
+ * starts.
+ * @param len the number of bytes in buffer which constitute the actual
+ * data.
+ * @return the RTP timestamp, or -1 in case of an error.
+ */
+ public static long getTimestamp(byte[] buf, int off, int len)
+ {
+ if (buf == null || Math.min(buf.length, len) < off + 12)
+ {
+ return -1;
+ }
+
+ return (((buf[off + 8] & 0xff) << 24)

Can we abstract this? Maybe use RawPacket.readInt()?

···


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/181/files/087306834c35d5ac0153ebc51f49037c5d6d38ef..061ccaa92c90fab568d16cdb9360b419aa201457#r74089145


#18

+ * Gets the RTCP sender SSRC.
+ *
+ * @param buf the byte buffer that contains the RTCP header.
+ * @param off the offset in the byte buffer where the RTCP header starts.
+ * @param len the number of bytes in buffer which constitute the actual
+ * data.
+ * @return the unsigned RTCP sender SSRC, or -1 in case of an error.
+ */
+ public static long getSenderSSRC(byte[] buf, int off, int len)
+ {
+ if (buf == null || Math.min(buf.length, len) < off + 8)
+ {
+ return -1;
+ }
+
+ return (((buf[off + 4] & 0xff) << 24)

Can we abstract this? Maybe use RawPacket.readInt()?

···


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/181/files/087306834c35d5ac0153ebc51f49037c5d6d38ef..061ccaa92c90fab568d16cdb9360b419aa201457#r74089234


#19

+ /**
+ * Sets the RTP timestamp.
+ *
+ * @param buf the byte buffer that contains the RTCP header.
+ * @param off the offset in the byte buffer where the RTCP sender info
+ * starts.
+ * @param len the number of bytes in buffer which constitute the actual
+ * data.
+ * @param ts the new timestamp to be set.
+ *
+ * @return the number of bytes written.
+ */
+ public static int setTimestamp(
+ byte[] buf, int off, int len, long ts)
+ {
+ if (buf == null || Math.min(buf.length, len) < off + 12)

Same as above

···


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/181/files/087306834c35d5ac0153ebc51f49037c5d6d38ef..061ccaa92c90fab568d16cdb9360b419aa201457#r74089285


#20

+ * @return the number of bytes written.
+ */
+ public static int setTimestamp(
+ byte[] buf, int off, int len, long ts)
+ {
+ if (buf == null || Math.min(buf.length, len) < off + 12)
+ {
+ return -1;
+ }
+
+ buf[off + 8] = (byte)(ts>>24);
+ buf[off + 9] = (byte)(ts>>16);
+ buf[off + 10] = (byte)(ts>>8);
+ buf[off + 11] = (byte)ts;
+
+ return 12;

In what sense have we written 12 bytes? Given the current usage of this, it would be better to declare this void and return 'true' in processRTCP()

···


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/181/files/087306834c35d5ac0153ebc51f49037c5d6d38ef..061ccaa92c90fab568d16cdb9360b419aa201457#r74090019