[jitsi-dev] [jitsi/jitsi-videobridge] fix(ls-hack): Sends 3 waves of black key frames. (#349)


#1

Sends 3 waves of black key frames, each separated by 1 second, to make
sure that at least one black key frame reaches the receiver after
signaling has propagated.
You can view, comment on, or merge this pull request online at:

  https://github.com/jitsi/jitsi-videobridge/pull/349

-- Commit Summary --

  * fix(ls-hack): Sends 3 waves of black key frames.

-- File Changes --

    M src/main/java/org/jitsi/videobridge/LipSyncHack.java (55)

-- Patch Links --

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


#2

This was not introduced with this PR but needs fixing: the catch block on line 494 decrements injectState.numOfKeyframesSent, but the increment itself is inside the try block.

···

--
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/jitsi-videobridge/pull/349#issuecomment-255902093


#3

bgrozev commented on this pull request.

         {

- this.scheduledFuture = scheduler.scheduleAtFixedRate(
- this, WAIT_MS , KEY_FRAME_RATE_MS, TimeUnit.MILLISECONDS);
+ synchronized (injectState)
+ {
+ if (!injectState.active
+ || injectState.numOfKeyframesSent >= MAX_KEY_FRAMES)
+ {
+ return -1L;

It is not clear from the documentation of RecurringRunnable what the semantics of returning -1 are, but from a quick look at the code it looks like this would make RecurringRunnableExecutor run the task perpetually.

···

--
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/jitsi-videobridge/pull/349#pullrequestreview-5565068


#4

bgrozev commented on this pull request.

         {

- this.scheduledFuture = scheduler.scheduleAtFixedRate(
- this, WAIT_MS , KEY_FRAME_RATE_MS, TimeUnit.MILLISECONDS);
+ synchronized (injectState)
+ {
+ if (!injectState.active
+ || injectState.numOfKeyframesSent >= MAX_KEY_FRAMES)
+ {
+ return -1L;
+ }

···

+
+ // send 3 "waves" of 10 packet bursts every 1 second. This makes
+ // sure that the signaling has propagated and that eventually a
+ // packet gets through SRTP.
+ long delay = 0;
+ if (injectState.numOfKeyframesSent % 10 == 0)

RecurringRunnableExecutor calls getTimeUntilNextRun() before it calls run() and injectState.numOfKeyframesSent is initialized to 0. Wouldn't this cause an initial delay of WAIT_MS?

--
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/jitsi-videobridge/pull/349#pullrequestreview-5565457


#5

bgrozev commented on this pull request.

         {

- this.scheduledFuture = scheduler.scheduleAtFixedRate(
- this, WAIT_MS , KEY_FRAME_RATE_MS, TimeUnit.MILLISECONDS);
+ synchronized (injectState)
+ {
+ if (!injectState.active
+ || injectState.numOfKeyframesSent >= MAX_KEY_FRAMES)
+ {
+ return -1L;
+ }

···

+
+ // send 3 "waves" of 10 packet bursts every 1 second. This makes
+ // sure that the signaling has propagated and that eventually a
+ // packet gets through SRTP.
+ long delay = 0;
+ if (injectState.numOfKeyframesSent % 10 == 0)

Nevermind, it wouldn't because of the clause below. Can we check "lastRunTime < 0L" in the beginning of the method and return 0 immediately, though?

--
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/jitsi-videobridge/pull/349


#6

gpolitis commented on this pull request.

         {

- this.scheduledFuture = scheduler.scheduleAtFixedRate(
- this, WAIT_MS , KEY_FRAME_RATE_MS, TimeUnit.MILLISECONDS);
+ synchronized (injectState)
+ {
+ if (!injectState.active
+ || injectState.numOfKeyframesSent >= MAX_KEY_FRAMES)
+ {
+ return -1L;

You’re right, this needs a little more work. I should de-register the recurring runnable.

···

--
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/jitsi-videobridge/pull/349


#7

gpolitis commented on this pull request.

         {

- this.scheduledFuture = scheduler.scheduleAtFixedRate(
- this, WAIT_MS , KEY_FRAME_RATE_MS, TimeUnit.MILLISECONDS);
+ synchronized (injectState)
+ {
+ if (!injectState.active
+ || injectState.numOfKeyframesSent >= MAX_KEY_FRAMES)
+ {
+ return -1L;
+ }

···

+
+ // send 3 "waves" of 10 packet bursts every 1 second. This makes
+ // sure that the signaling has propagated and that eventually a
+ // packet gets through SRTP.
+ long delay = 0;
+ if (injectState.numOfKeyframesSent % 10 == 0)

I want to wait for an initial WAIT_MS, so the fact that we don't is a bug. Thanks for catching 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/jitsi-videobridge/pull/349


#8

gpolitis commented on this pull request.

         {

- this.scheduledFuture = scheduler.scheduleAtFixedRate(
- this, WAIT_MS , KEY_FRAME_RATE_MS, TimeUnit.MILLISECONDS);
+ synchronized (injectState)
+ {
+ if (!injectState.active
+ || injectState.numOfKeyframesSent >= MAX_KEY_FRAMES)
+ {
+ return -1L;
+ }

···

+
+ // send 3 "waves" of 10 packet bursts every 1 second. This makes
+ // sure that the signaling has propagated and that eventually a
+ // packet gets through SRTP.
+ long delay = 0;
+ if (injectState.numOfKeyframesSent % 10 == 0)

The idea for waiting an initial WAIT_MS was to wait for actual video packets from the sender, but we risk of clipping the first few seconds of audio, in case the sender is video muted. I think I’m going to remove this initial wait.

--
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/jitsi-videobridge/pull/349


#9

@gpolitis pushed 1 commit.

3d6465c fix(lshack): Takes care of some edge cases.

···


You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/349/files/9e8c97c5665e9e7a99e28fcbca818b9f4323a5b4..3d6465cbd42d63ce84c870966d16f43b8c0dae96


#10

bgrozev commented on this pull request.

@@ -21,10 +21,10 @@

import org.jitsi.impl.neomedia.rtp.*;
import org.jitsi.service.neomedia.*;
import org.jitsi.util.*;
+import org.jitsi.util.concurrent.*;

import java.lang.ref.*;

This is now an unused import, can you please remove 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/jitsi-videobridge/pull/349#pullrequestreview-5732085


#11

bgrozev commented on this pull request.

             states.put(receiveVideoSSRC, injectState);

             InjectTask injectTask = new InjectTask(injectState);
- injectTask.schedule();
+ scheduler.registerRecurringRunnable(injectTask);

···

+
+ if (DEBUG)
+ {
+ logger.debug("ls_hack_rerister,ssrc=" + injectState.ssrc);

Probably not important, but is that supposed to say "register"?

--
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/jitsi-videobridge/pull/349#pullrequestreview-5732103


#12

bgrozev commented on this pull request.

@@ -443,21 +450,41 @@ public void run()

                 if (!injectState.active
                     >> injectState.numOfKeyframesSent >= MAX_KEY_FRAMES)
                 {
- scheduledFuture.cancel(true);
+ deregister("completed");
+ return;
+ }

···

+
+ if (endpoint == null)
+ {
+ deregister("endpoint_expired");

Should this also check for expiration (like the one for the channel below)?

--
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/jitsi-videobridge/pull/349#pullrequestreview-5732155


#13

bgrozev commented on this pull request.

+

+ return timeUntilNextRun;
+ }
+ }

···

+
+ /**
+ * De-registers this {@code RecurringRunnable} and optionally prints a
+ * debug message.
+ *
+ * @param reason the de-registration reason
+ */
+ private void deregister(String reason)
+ {
+ if (DEBUG)
+ {
+ logger.debug("ls_hack_dererister"

s/dererister/deregister /

--
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/jitsi-videobridge/pull/349#pullrequestreview-5732216


#14

@gpolitis pushed 1 commit.

00fa2f4 style(lshack): Removes unused imports and fixes comment types.

···


You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/349/files/3d6465cbd42d63ce84c870966d16f43b8c0dae96..00fa2f4843e4dc32c75444475baf3e8ed57fb14e


#15

Jenkins: 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/jitsi-videobridge/pull/349#issuecomment-256168313


#16

@gpolitis pushed 1 commit.

a16c430 fix(lshack): Adds missing import.

···


You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/349/files/00fa2f4843e4dc32c75444475baf3e8ed57fb14e..a16c430a0221472298c0a1f60a38c4bbe05c838a


#17

Merged #349.

···

--
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/jitsi-videobridge/pull/349#event-836145393