Add pause/resume recording functionality to Record&Play and SIP plugins

[isnumanagic]

This PR adds pause/resume functionality to Janus recorder, and implements signaling for it in Record&Play and SIP plugins.

This functionality is useful when relaying sensitive information during recorded meeting. It is implemented by skipping writing of RTP packets during the pause duration. On resume we request PLI to fix any video artifacts that can occur due to skipped frames in recording.

[lminiero]

I remember this was attempted already in #918, which I had several doubts on. I think your patch has a similar issue in how recordings are handled, because the way you're doing it in SIP and Record&Play this will simply introduce big holes in the recording when postprocessing: when you pause and resume after a while, packets will have huge timestamp/seq number changes, which will translate to either a ton of silence, or missing video.

[isnumanagic]

Yes, that is by design.

[isnumanagic]

Perhaps this can be ammended by writing silence timespans into the recording header, so that after or during post processing those segments can be skipped?

[lminiero]

Yes, that is by design.

I don't think that's correct. A pause/resume should be seamless in the recording too.

Perhaps this can be ammended by writing silence timespans into the recording header, so that after or during post processing those segments can be skipped?

MJR headers can't be edited after they're written, and I don't like the idea of making a lot of changes to janus-pp-rec to accomodate this.

[isnumanagic]

I see the packet timestamps are calculated by recorder, then the solution can be to track the total paused time and subtracting it when calculating timestamps? This should make the pause seamless in both playing and postprocessing.

[lminiero]

The solution is to write packets to the recorder that have consistent RTP headers, as we do in other plugins e.g. for source switching and RTP forwarders.

[isnumanagic]

Going through the code i see you use janus_rtp_switching_context struct for tracking timestamps and seq-numbers, and janus_rtp_header_update method to make changes to header. I'll try to do the same here and will update the PR.

[lminiero]

My guess is that, once you add the context in the mix, you can just trigger its behaviour by changing its SSRC before a resume (which will trick it into thinking the source changed and so headers need to be updated).

[isnumanagic]

Updated PR with requested changes. Just changing the ssrc did not work - I had to add time offset information.

[lminiero]

Updated PR with requested changes. Just changing the ssrc did not work - I had to add time offset information.

From a quick glance to the code I don't think you're using the RTP context correctly. Timestamp offset management is already done automatically by RTP contexts, when the SSRC changes: it looks to me like you're just resetting the sequence number instead, which is why you're forced to manually fix the timestamp. When recording is resumed, it's the SSRCs in the context you have to change (a_last_ssrc, v_last_ssrc), not the one in the packet: this way, even if the packet is using the same SSRC it always used, the context will think it changed, and it will trigger the rewriting logic.

[isnumanagic]

I tried to just reset the ssrc by changing the one in the context, but as I said it does not work - the silence during the pause duration remained in the recording. This is because the base timestamp is reset, but the offset is then calcualted and applied (denoted by the How much time since the last audio RTP packet? We compute an offset accordingly comment). That is why i reset the seq numbering instead of entire ssrc and adjust the timestamp.

[lminiero]

Ah I think I understand what you mean now: the SSRC change forces sequence number and timestamp to be updated, but the timestamp will take into account the time that passed, and so a lot of silence will be inserted, which shouldn't indeed happen in this case. That said, I don't think the way you're updating the timestamp offset works, and I don't like the idea of adding new properties to an already crowded structure. It should be possible to do this with the properties already there.

[isnumanagic]

I have updated CR as requested. I have managed to remove the silence between pauses by overwriting a/v_last_time properties. I have removed a/v_time_offset properties I have added to switching context, but kept a/v_ts_reset flags as it is more descriptive to do seq_reset and ts_reset on resume than to change the ssrc, but I can roll back the flags as well if needed.

[lminiero]

Thanks! I'll need time to review, since your changes modifies the code of the context update method as well, which can have a huge impact (we use it pretty much everywhere).

[isnumanagic]

The context update method has not been functionally changed, I just extracted the seq_reset part (which was already extracted actually, i just removed duplicated code) and ts_reset part of ssrc reset into separate if branches for finer control.

[isnumanagic]

So basically:

if(ssrc != context->a_last_ssrc) {
  ...
  // ts_reset code
  // seq_reset code
  ...
}

became:

if(ssrc != context->a_last_ssrc) {
  ...
  context->a_ts_reset = TRUE;
  context->a_seq_reset = TRUE;
  ...
}
if(context->a_ts_reset) {
  // ts_reset code
}
if(context->a_seq_reset) {
  // seq_reset code
}

[isnumanagic]

Hey @lminiero, if you have the time please take a look.

[lminiero]

It's IETF week so not sure when I'll be able to do a deep dive. I'll try to add some comments to the PR when I can.

[lminiero]

I had another look at the changes: please notice I haven't tested this yet, which will probably not happen before next week.

[isnumanagic]

Updated PR with requested changes.

[isnumanagic]

Hey @lminiero, any updates?

[lminiero]

@isnumanagic apologies, I went on vacation and forgot about this 🤭
I'll have a look at the fixes you made at the time and let you know. Since this has an impact on how mjr files are created (the extra RTP context that is part of the code is a big change) we'll need time to do proper testing anyway, as we use recordings a lot.

[lminiero]

Took me a while, but I found some time to test this: it seemed to work as expected for the brief test I made, even though I didn't push it much. I did notice some UI things, though (pause button still visible when replaying recordings), and some messaging/state things related to how pause/resume are handled that may need fixing (I'll try to make some tests for that too).

[isnumanagic]

Uodated PR with requested changes

[lminiero]

Thanks for the quick fixes! I added a couple more notes: I think we're close to merging this ✌️

[isnumanagic]

Updated PR with requested changes

[lminiero]

LGTM now, thanks for all the fixes! Pinging @atoppi to give a review as well (in case I missed anything), and @alexamirante to check if this might cause issues with the way we handle recordings in production.

[atoppi]

this looks good, I just don't get why we need the ts_reset flag, is this just a design choice?

[atoppi]

left my only question as a separate comment ^

[isnumanagic]

@atoppi It was a design choice. It's more intuitive to set ts_reset and seq_reset flags when resuming recording, than to manually change SSRC, expecting it to reset timestamp and sequence number.

[atoppi]

ok, thanks or clarifying, LGTM 👍

[lminiero]

Ack, merging then 👍 Thanks for the patience!

[lminiero]

@isnumanagic you created a monster: I'm way too often using the pause/resume feature just to do funny faces! I see a potential for doing TikTok like videos with WebRTC 🤣