[jitsi-dev] [jitsi/jitsi-videobridge] [Jisti-Bridge] Move the RoC logic to videoChannel. (#241)


#1

Summary: SRTP logic to detecting ROC doesn't quiet work when streams are paused (due to speech activity) and once a stream resumes, it may fail to guess the correct ROC, causing rtp packet drops. Jitsi already had a logic for handing this (sending a few rtp packets every once in a while) but not general enough for our usecase. In this CR, I moved that logic to videoChannel to take advantage of LastNController

Test Plan: tested locally

Reviewers: brian

Subscribers: eng-team-list

Differential Revision: https://phab.fatline.io/D3263
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * [Jisti-Bridge] Move the RoC logic to videoChannel.

-- File Changes --

    M src/main/java/org/jitsi/videobridge/VideoChannel.java (101)
    M src/main/java/org/jitsi/videobridge/simulcast/sendmodes/SwitchingSendMode.java (93)

-- Patch Links --

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


#2

Hi, thanks for your contribution!
If you haven't already done so, could you please make sure you sign our CLA (https://jitsi.org/icla for individuals and https://jitsi.org/ccla for corporations)? We would unfortunately be unable to merge your patch unless we have that piece :(.

···

---
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/241#issuecomment-218539738


#3

i think maryam may already be covered under the CLA (i either did the entire company or added her specifically maybe?). if not let us know.

···

---
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/241#issuecomment-218542927


#4

ok, maryam did an individual cla but i also just did a corporate one.

···

---
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/241#issuecomment-218577943


#5

@@ -19,7 +19,10 @@
import java.io.*;
import java.net.*;
import java.util.*;
+import java.util.concurrent.ConcurrentHashMap;

Could you please use package imports (according to the Jitsi Code Convention)? Thank you!

···

---
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/241/files/36b6741489e8d329b936688c25f3a13b8806d9c7#r62919909


#6

I think jenkins only checks its whitelist and not the CLA list.

Jenkins: add to whitelist

···

---
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/241#issuecomment-218595427


#7

This changes the key used for the counters from the RTP SSRC of the packet to the endpoint ID. The ROC in SRTP is separate for each SSRC, so we should keep using the SSRC as the key.

Also I might be missing something here, but doesn't removing the code from SwitchingSendMode re-introduce the original problem? The switching mode is still is use, so we shouldn't break it. I think that keeping the code in both places would at least fix the problem when either simulcast in switching mode or last N (but not both) are used, and it seems like a reasonable solution.

cc @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/jitsi-videobridge/pull/241#issuecomment-218777495


#8

@bgrozev is right that removing the code from SwitchingSendMode breaks that mode.

···

---
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/241#issuecomment-218792802


#9

@bgrozev and @gpolitis Based on my understanding, SwitchingSendMode is at the layer below. SendMode (either one of the send modes, rewriting or switching) is part of of simulcastSender. The lastN logic and sendMode are both taken into account when we are at the channel level. The downside of having it at this central place is SSRC are masked out and we only have endPoints/sourceId but I don't see any obvious problem with that since there are always one SSRC per endPoint?

what do you guys think? Of course having the logic at two places (rewriting and switching) also works but it has redundancy in 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/241#issuecomment-218821785


#10

The lastN logic and sendMode are both taken into account when we are at the channel level.

For a packet to reach the receiver, it needs to pass through both the channel logic and the sendMode logic. In the case of switching mode, the VideoChannel logic will let some packets through, but the SwitchingSendMode (without the additional logic) will drop them.

The downside of having it at this central place is SSRC are masked out and we only have endPoints/sourceId but I don't see any obvious problem with that since there are always one SSRC per endPoint?

I don't think it's a good idea to assume that each endpoint only has a single SSRC (or a single SSRC for video/audio). It should be a simple fix though, since in VideoChannel we have the actual bytes of the packet, so we can read the SSRC (with e.g. RawPacket.getSSRC).

···

---
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/241#issuecomment-218836104


#11

Hi @mdaneshi, allowing a few packets every once in a while will mess-up the RTCP statistics. @bgrozev suggested (and I agree) that we could rewrite the sequence numbers early on, in the `VideoChannel.rtpTranslatorWillWrite` method, so that stream pauses are completely transparent to the rest of the JVB transform engines.

In particular, when a source is entering the _paused_ state, you keep the last sequence number that was forwarded for that source. When a source enters the _streaming_ state, the code continues from that last sequence number that was forwarded.

We don't care about reverse RTCP termination. The only thing that would require reverse transformation of sequence numbers are NACKs, but we have NACK termination for that.

Do you think that makes sense?

cc @brianh5

···

---
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/241#issuecomment-219088083


#12

makes sense...i think we'll live with this change on our branch for a bit and will come back around and get the rewriting stuff done when we get some time. thanks george.

···

---
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/241#issuecomment-219112057


#13

Closing (a solution was merged with #261)

···

---
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/241#issuecomment-228905295


#14

Closed #241.

···

---
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/241#event-705661037