[jitsi-dev] [jitsi/libjitsi] Implements transactional key frame requests. (#145)


#1

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

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

-- Commit Summary --

  * Implements transactional key frame requests.

-- File Changes --

    M src/org/jitsi/impl/neomedia/codec/video/Utils.java (7)
    M src/org/jitsi/impl/neomedia/rtp/translator/RTCPFeedbackMessageSender.java (303)
    M src/org/jitsi/impl/neomedia/rtp/translator/RTPTranslatorImpl.java (22)

-- Patch Links --

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


#2

@@ -36,8 +36,13 @@
     /**
      * Utility method that determines whether or not a packet is a key frame.
      */
- public static boolean isKeyFrame(RawPacket pkt, Byte redPT, byte vp8PT)
+ public static boolean isKeyFrame(RawPacket pkt, Byte redPT, Byte vp8PT)

if this function is vp8 specific, why not have it in the vp8 subdir?

···

---
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/145/files/e85d6828b51aaa87cd14b3536e714995f16225fc#r59802296


#3

@brianh5 agreed. we should make sure that at least the part that assumes VP8 is properly abastracted.

···

---
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/145#issuecomment-210325601


#4

Just to be clear: I don't think we need to block this until we do, but we should add it on the next day todo lists.

···

---
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/145#issuecomment-210325777


#5

@@ -36,8 +36,13 @@
     /**
      * Utility method that determines whether or not a packet is a key frame.
      */
- public static boolean isKeyFrame(RawPacket pkt, Byte redPT, byte vp8PT)
+ public static boolean isKeyFrame(RawPacket pkt, Byte redPT, Byte vp8PT)

Thanks @brianh5, you're right, this should be in the vp8 subdir.

···

---
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/145/files/e85d6828b51aaa87cd14b3536e714995f16225fc#r59893243


#6

@@ -225,6 +227,12 @@ public static int readUnsignedShort(byte[] buf, int off)
     private long localSSRC = -1;

     /**
+ *
+ */
+ private final RecurringProcessibleExecutor recurringProcessibleExecutor

@gpolitis, please add javadocs on the RecurringProcessibleExecutor field and getter. I know what RecurringProcessibleExecutor does in general but I'd like it to be obvious why RTPTranslatorImpl is a provider of RecurringProcessibleExecutor.

···

---
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/145/files/719acf992feeed99ff67e762ae5264f8cff3f82d#r59897749


#7

@@ -225,6 +227,12 @@ public static int readUnsignedShort(byte[] buf, int off)
     private long localSSRC = -1;

     /**
+ *
+ */
+ private final RecurringProcessibleExecutor recurringProcessibleExecutor

Will do, thank you @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/145/files/719acf992feeed99ff67e762ae5264f8cff3f82d#r59897907


#8

@@ -585,6 +593,11 @@ int didRead(
                 int pt = buf[off + 1] & 0x7f;

                 format = streamRTPManager.getFormat(pt);
+
+ // Pass the packet to the feedback message sender to update
+ // its transactions.
+ rtcpFeedbackMessageSender.maybeStopRequesting(
+ streamRTPManager, ssrc, pt, buf, off, len);

@gpolitis, "Pass the packet to the feedback message sender" is what the source code says so I find it stated in the comment unhelpful. I would like to read in the comment why it's necessary and, while "update its transactions" is in that spirit, I have no idea what transactions means. I guess I expect something more accessible. Anyway, that's my opinion and you don't have to act on 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/145/files/719acf992feeed99ff67e762ae5264f8cff3f82d#r59898454


#9

@@ -585,6 +593,11 @@ int didRead(
                 int pt = buf[off + 1] & 0x7f;

                 format = streamRTPManager.getFormat(pt);
+
+ // Pass the packet to the feedback message sender to update
+ // its transactions.
+ rtcpFeedbackMessageSender.maybeStopRequesting(
+ streamRTPManager, ssrc, pt, buf, off, len);

Will do, thank you @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/145/files/719acf992feeed99ff67e762ae5264f8cff3f82d#r59898580


#10

@@ -40,11 +72,17 @@
     private final RTPTranslatorImpl rtpTranslator;

     /**
- * The next (command) sequence numbers to be used for RTCP feedback message
- * packets per SSRC of packet sender-SSRC of media source pair.
+ * The {@link RecurringProcessibleExecutor} which will periodically call
+ * call {@link KeyframeRequester#process()} and trigger their retry logic.

will periodically call call

Could you please remove the extra occurrence of the word call, @gpolitis?

···

---
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/145/files/c99137cfdb87be24727833638ad618c5781b6756#r59925511


#11

      *
      * @param mediaSenderSSRC the SSRC of the media sender/source
      * @return <tt>true</tt> if a FIR command was sent; otherwise,
      * <tt>false</tt>
      */
     public boolean sendFIR(int mediaSenderSSRC)
     {
- boolean sentFIR = false;
- /*
- * XXX Currently, the method effectively broadcasts a FIR i.e. sends it
- * to all streams connected to rtpTranslator because MediaStream's
- * method getRemoteSourceIds returns an empty list (possibly due to
- * RED).
- */
- for (StreamRTPManager streamRTPManager
- : rtpTranslator.getStreamRTPManagers())
- {
- MediaStream stream = streamRTPManager.getMediaStream();
+ // It's OK for this method to be a little "slow" (so that the
+ // {@code RTCPFeedbackMessageSender#maybeStopRequesting} is lock-less).
+ KeyframeRequester oldRequester = kfRequesters.putIfAbsent(
+ mediaSenderSSRC, new KeyframeRequester(mediaSenderSSRC));

I'm afraid I don't get it... You're going to initialize a new KeyframeRequester instance each time you want to send a FIR even if you don't really need that instance? I don't feel comfortable about that. Could you please explain whether that's 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/145/files/c99137cfdb87be24727833638ad618c5781b6756#r59925997


#12

      *
      * @param mediaSenderSSRC the SSRC of the media sender/source
      * @return <tt>true</tt> if a FIR command was sent; otherwise,
      * <tt>false</tt>
      */
     public boolean sendFIR(int mediaSenderSSRC)
     {
- boolean sentFIR = false;
- /*
- * XXX Currently, the method effectively broadcasts a FIR i.e. sends it
- * to all streams connected to rtpTranslator because MediaStream's
- * method getRemoteSourceIds returns an empty list (possibly due to
- * RED).
- */
- for (StreamRTPManager streamRTPManager
- : rtpTranslator.getStreamRTPManagers())
- {
- MediaStream stream = streamRTPManager.getMediaStream();
+ // It's OK for this method to be a little "slow" (so that the
+ // {@code RTCPFeedbackMessageSender#maybeStopRequesting} is lock-less).
+ KeyframeRequester oldRequester = kfRequesters.putIfAbsent(
+ mediaSenderSSRC, new KeyframeRequester(mediaSenderSSRC));

I wanted to use a ConcurrentHashMap instead of a synchronized block (which would also have to be present in the `maybeStopRequesting` that runs for every incoming RTP packet). My thinking was that sending FIRs is much less often than RTP packets, so it's okay for the sendFIR method to be slow. I don't mind changing this, if you prefer it the other way.

···

---
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/145/files/c99137cfdb87be24727833638ad618c5781b6756#r59926535


#13

         {
- if (sendFIR(destination, mediaSenderSSRC))
- sentFIR = true;
+ super(FIR_RETRY_INTERVAL_MS);
+ this.mediaSenderSSRC = mediaSenderSSRC;
+ this.sequenceNumber = new AtomicInteger(0);

I'm going to make a very moot point here, I admit. But isn't that easier to read if you initialize sequenceNumber above given that it's final and doesn't get initialized with a constructor argument?

···

---
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/145/files/c99137cfdb87be24727833638ad618c5781b6756#r59926725


#14

@@ -40,11 +72,17 @@
     private final RTPTranslatorImpl rtpTranslator;

     /**
- * The next (command) sequence numbers to be used for RTCP feedback message
- * packets per SSRC of packet sender-SSRC of media source pair.
+ * The {@link RecurringProcessibleExecutor} which will periodically call
+ * call {@link KeyframeRequester#process()} and trigger their retry logic.

Will do, thank you @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/145/files/c99137cfdb87be24727833638ad618c5781b6756#r59929709


#15

         {
- if (sendFIR(destination, mediaSenderSSRC))
- sentFIR = true;
+ super(FIR_RETRY_INTERVAL_MS);
+ this.mediaSenderSSRC = mediaSenderSSRC;
+ this.sequenceNumber = new AtomicInteger(0);

Will do, thank you @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/145/files/c99137cfdb87be24727833638ad618c5781b6756#r59929722


#16

      *
      * @param mediaSenderSSRC the SSRC of the media sender/source
      * @return <tt>true</tt> if a FIR command was sent; otherwise,
      * <tt>false</tt>
      */
     public boolean sendFIR(int mediaSenderSSRC)
     {
- boolean sentFIR = false;
- /*
- * XXX Currently, the method effectively broadcasts a FIR i.e. sends it
- * to all streams connected to rtpTranslator because MediaStream's
- * method getRemoteSourceIds returns an empty list (possibly due to
- * RED).
- */
- for (StreamRTPManager streamRTPManager
- : rtpTranslator.getStreamRTPManagers())
- {
- MediaStream stream = streamRTPManager.getMediaStream();
+ // It's OK for this method to be a little "slow" (so that the
+ // {@code RTCPFeedbackMessageSender#maybeStopRequesting} is lock-less).
+ KeyframeRequester oldRequester = kfRequesters.putIfAbsent(
+ mediaSenderSSRC, new KeyframeRequester(mediaSenderSSRC));

You're right, I now synchronize the writers to that map and keep the readers lock-less.

···

---
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/145/files/c99137cfdb87be24727833638ad618c5781b6756#r59929910


#17

+ {
+ vp8PT = entry.getKey();
+ }
+ else if (Constants.RED.equals(encoding))
+ {
+ redPT = entry.getKey();
+ }
+ }
+
+ if (vp8PT == null || vp8PT != pt)
+ {
+ return;
+ }
+
+ RawPacket pkt = new RawPacket(buf, off, len);
+ if (!Utils.isKeyFrame(pkt, redPT, vp8PT))

I get it that the JRE is optimized for small short-lived allocations. But I still don't feel absolutely at piece with allocating a new RawPacket just to pass it to a util and then leave it to the garbage collector... In my mind, having an overloaded Utils.isKeyFrame which takes byte[],int,int (in addition to the existing one which takes RawPacket) is OK. Anyway, I don't want you to do anything and I merely wanted to share what's on my mind.

···

---
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/145/files/ec40e962eda2c9894b7c1ed6162557532ffac13a#r59931171


#18

+ {
+ vp8PT = entry.getKey();
+ }
+ else if (Constants.RED.equals(encoding))
+ {
+ redPT = entry.getKey();
+ }
+ }
+
+ if (vp8PT == null || vp8PT != pt)
+ {
+ return;
+ }
+
+ RawPacket pkt = new RawPacket(buf, off, len);
+ if (!Utils.isKeyFrame(pkt, redPT, vp8PT))

You're right, it doesn't make any sense to allocate a `RawPacket` here. Thank you @lyubomir, I'll fix 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/145/files/ec40e962eda2c9894b7c1ed6162557532ffac13a#r59931544


#19

Merged #145.

···

---
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/145#event-629527023


#20

      *
      * @param mediaSenderSSRC the SSRC of the media sender/source
      * @return <tt>true</tt> if a FIR command was sent; otherwise,
      * <tt>false</tt>
      */
     public boolean sendFIR(int mediaSenderSSRC)
     {
- boolean sentFIR = false;
- /*
- * XXX Currently, the method effectively broadcasts a FIR i.e. sends it
- * to all streams connected to rtpTranslator because MediaStream's
- * method getRemoteSourceIds returns an empty list (possibly due to
- * RED).
- */
- for (StreamRTPManager streamRTPManager
- : rtpTranslator.getStreamRTPManagers())
+ boolean registerRecurringProcessible = false;
+ synchronized (kfRequesters)

Could synchronization be removed by replacing this line and the following "if" with a call to atomic putIfAbsent(...)?

···

---
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/145/files/7cdebacba6539fab0a9ae8a2f715889223009ce9#r59975729