[jitsi-dev] [jitsi/jitsi-videobridge] Fix #227: Fix SSRC Mapping when people join a recorded conversation (#240)


#1

Add the mapping from SSRC to EndpointID to the Synchronizer instance used by
RecorderRtpImpl when new SSRCs are signalled over colibri.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Fix #227: Fix SSRC Mapping when people join a recorded conversation

-- File Changes --

    M src/main/java/org/jitsi/videobridge/RtpChannel.java (25)

-- Patch Links --

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


#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/240#issuecomment-218525552


#3

It is signes now

···

---
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/240#issuecomment-218527166


#4

ping @gavllew

···

--
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/240#issuecomment-241006463


#5

I can't help much, as I'm just another third party - I can't approve pull requests. However I do at least have a comment on the diff which I'll add in a second.

···

--
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/240#issuecomment-241009356


#6

             for (Integer addedSSRC : addedSSRCs)
             {
                 try
                 {
                     // Do allow the number of explicitly signalled SSRCs to
                     // exceed the limit.
                     addReceiveSSRC(addedSSRC, false);
+
+ // If recording is enabled, we need to add the mapping from
+ // SSRC to EndpointID into the Synchronizer
+ // instance used by RecorderRtpImpl.
+ if (addToSyncronizer)
+ {
+ synchronizer.setEndpoint(addedSSRC & 0xffffffffl,

The `synchronizer` variable will be out-of-scope before you reach this code 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/240/files/7c0cd013ba7d0beaa1eab9ccfbd67f5e0f92aad8#r75475536


#7

Looks OK to me, except that it doesn't compile :). Should be a quick fix.

Are you actually using some variant of this? I don't understand what problem it solves. If I understand correctly, it only makes a difference when recording SSRCs that are signaled, but don't send and RTP/RTCP packets.

···

--
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/240#issuecomment-241054586


#8

The core problem was that new streams added after recording had started were not appearing in metadata.json if the SSRCs were signalled before arriving in media packets. This is because the metadata attempts to calculate the wall-clock instant that the stream starts, which depends on the per-Endpoint RTP timestamp mapping maintained by the Synchronizer.

Xura is using a variant of this fix, but we're currently unable to contribute anything because we've never managed to get the CLA sorted out. :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/jitsi-videobridge/pull/240#issuecomment-241070748


#9

I see now that the code in acceptDataInputStreamDatagramPacket() only updates the synchronizer if the SSRC is new. Thanks for the explanation.

We have the CLA for @roidelapluie so we can get this merged when the PR is fixed.

···

--
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/240#issuecomment-241073423


#10

updated

···

--
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/240#issuecomment-241182240


#11

             for (Integer addedSSRC : addedSSRCs)
             {
                 try
                 {
                     // Do allow the number of explicitly signalled SSRCs to
                     // exceed the limit.
                     addReceiveSSRC(addedSSRC, false);
+
+ // If recording is enabled, we need to add the mapping from
+ // SSRC to EndpointID into the Synchronizer
+ // instance used by RecorderRtpImpl.
+ if (addToSyncronizer)
+ {
+ synchronizer.setEndpoint(addedSSRC & 0xffffffffl,

fixed

···

--
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/240/files/7c0cd013ba7d0beaa1eab9ccfbd67f5e0f92aad8#r75573487


#12

@@ -1726,13 +1726,38 @@ public void setSources(List<SourcePacketExtension> sources)
         addedSSRCs.removeAll(oldSignaledSSRCs);
         if (!addedSSRCs.isEmpty())
         {
+
+ boolean addToSyncronizer = false;
+ Recorder recorder = getContent().getRecorder();

Calling getRecorder will create a Recorder instance if one doesn't exist (regardless of whether recording is enabled or not). It should only be called if isRecording() returns true (yeah, the design of the API is not good...).

···

--
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/240/files/0925a0c6f744fe50b088c52d72dc53c1ff01488e#r75583665


#13

@@ -1726,13 +1726,38 @@ public void setSources(List<SourcePacketExtension> sources)
         addedSSRCs.removeAll(oldSignaledSSRCs);
         if (!addedSSRCs.isEmpty())
         {
+
+ boolean addToSyncronizer = false;
+ Recorder recorder = getContent().getRecorder();

Fixed

···

--
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/240/files/0925a0c6f744fe50b088c52d72dc53c1ff01488e#r75584569


#14

Merged #240.

···

--
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/240#event-764554793


#15

@zbettenbuk this looks like it may solve one of the issues you described

···

--
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/240#issuecomment-241749006


#16

@bgrozev which one you mean?

···

--
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/240#issuecomment-241756882


#17

The missing entries from metadata.json

···

--
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/240#issuecomment-241759299