Added changes to pause & resume recorder #918

AjayChoudary · 2017-06-04T11:36:58Z

As @lminiero suggested in discussion , i created the pull request to pause & resume the active recording by manipulating RTP Time-stamps.

lminiero · 2017-06-05T10:16:31Z

Thanks for working on this! From a quick glance there are some things that need to be fixed, though:

There's a new "keyframe" request that is not needed. I see you already added an automatic FIR/PLI request when recording is resumed.
Pausing/resuming recordings don't check if the recording object exists, which should cause an error if not.
The ts-related changes in the recorder are not what I meant by keeping the RTP header consistent. That should not be done in the recorder code, but outside. Look at the calls to janus_rtp_header_update to see how plugins handle that for features like source switching: you'll see you'll need a context for the recorder, in case, and that's the plugin's responsibility, as the recorder should just worry about dumping packets without messing with them (and risking to break them).

AjayChoudary · 2017-06-05T15:15:44Z

@lminiero below are my comments

This "keyframe" is very handy for application servers, which are using rtp-forword. Instead of blindly requesting keyframe for every 10 seconds in janus, we can trigger keyframe request from App server whenever needed.
Sorry I didn't get, which part you are referring to. Mostly i added NULL checks :)
I have checked janus_rtp_header_update it only depends on SSRC, but in our case SSRC will be same. And we don't need to store all those context details.
Pause the recording is completely independent of plug-in(audio/video rooms or record&play).
I felt its good if we are doing recorder, post-processor is even better place.
In my use case, I am pausing all the conference handles, but if any one wants to pause only one handle then they need this info in post-processor for synchronizing with other streams.
At least for data channel we need to touch recorder.

lminiero · 2017-06-09T08:44:50Z

Just a ping to tell you I haven't forgotten about this PR... I'm just awfully busy this period and can't look into it yet, sorry. That said, janus_rtp_header_update is not only used for switching: if you simulate an SSRC change, it will force the whole context to self update, and so make the sequence number and timestamp immediately consistent. We use something like this for viewers pausing/unpausing in the VideoRoom too (although there we only update the sequence number but not the timestamp).

lminiero

Tried doing a deeper review. I'm not 100% sure this fits nicely in how we do things in all plugins and in the post-processor, but I'd have to try this. It definitely has interesting concepts, though, so in case I try to tackle this myself later on this would help greatly. Thanks again for working on this!

lminiero · 2017-06-20T10:39:19Z

record.h

@@ -48,6 +48,10 @@ typedef struct janus_recorder {
 	gint64 created;
 	/*! \brief Media this instance is recording */
 	janus_recorder_medium type;
+	/*! \brief Timestaps for pause and resume the recording */


pausing and resuming

lminiero · 2017-06-20T10:39:42Z

record.h

@@ -48,6 +48,10 @@ typedef struct janus_recorder {
 	gint64 created;
 	/*! \brief Media this instance is recording */
 	janus_recorder_medium type;
+	/*! \brief Timestaps for pause and resume the recording */
+	gint64 paused_ts, offset_ts;
+	/*! \brief Whether recording paused or not */


Whether the recording has been paused or not

lminiero · 2017-06-20T10:40:58Z

record.h

+ * @param[in] recorder The janus_recorder instance
+ * @param[in] boolean Whether to pause or resume
+ * @returns 0 in case of success, a negative integer otherwise */
+int janus_recorder_pause(janus_recorder *recorder, gboolean pause);


I'd rather have two separate methods, janus_recorder_pause and janus_recorder_resume, as from a semantic perspective a method called janus_recorder_pause that can be used to resume as well is counter-intuitive.

lminiero · 2017-06-20T10:41:49Z

plugins/janus_videoroom.c

@@ -506,6 +506,7 @@ typedef struct janus_videoroom_publisher {
 	gint64 fir_latest;	/* Time of latest sent FIR (to avoid flooding) */
 	gint fir_seq;		/* FIR sequence number */
 	gboolean recording_active;	/* Whether this publisher has to be recorded or not */
+	gboolean recording_pause;


This is nitpicking, I know, but recording_paused is a better name for this. Can you also add a comment on the side, as we do for the other properties?

lminiero · 2017-06-20T10:42:54Z

plugins/janus_videoroom.c

@@ -2699,7 +2699,18 @@ static void janus_videoroom_recorder_create(janus_videoroom_publisher *participa
 		}
 	}
 }
-
+static void janus_videoroom_recorder_pause(janus_videoroom_publisher *participant, gboolean paused) {


See below for the semantic names. In this case, to keep things compact, the name could be changed to janus_videoroom_recorder_status or something like this, in order to keep the boolean flag.

lminiero · 2017-06-20T10:44:07Z

plugins/janus_videoroom.c

@@ -3349,6 +3362,28 @@ static void *janus_videoroom_handler(void *data) {
 						}
 					}
 				}
+				if(record_pause) {


There is no check here on whether arc, vrc or drc exist. Trying to pause/resume a non existing recording should return an error, while in this case you're always succeeding no matter what.

This is important, because you may set a recording to paused and then start it, and the fact that both succeeded may make you think that you started the recording as paused, and you can resume it later, which is not the case.

lminiero · 2017-06-20T10:48:35Z

plugins/janus_videoroom.c

@@ -2964,6 +2975,7 @@ static void *janus_videoroom_handler(void *data) {
 				publisher->video_active = FALSE;
 				publisher->data_active = FALSE;
 				publisher->recording_active = FALSE;
+				publisher->recording_pause = FALSE;


This should be reset in hangup_media as well. As a side note, it might be useful to have this property returned when doing query_session as well, as we have a section related to recording in there when querying publishers.

lminiero · 2017-06-20T10:49:41Z

plugins/janus_videoroom.c

+					JANUS_LOG(LOG_VERB, "Video keyframe, sending PLI to %"SCNu64" (%s)\n",
+						participant->user_id, participant->display ? participant->display : "??");
+					gateway->relay_rtcp(participant->session->handle, 1, buf, 12);
+				}


Why is this request not generating any event to confirm the success?

lminiero · 2017-06-20T10:52:52Z

record.c

+			recorder->offset_ts = (uint32_t) recorder->offset_ts + (ntohl(rtp->timestamp) - (uint32_t)recorder->paused_ts);
+			JANUS_LOG(LOG_INFO, "RESUME TIME TS = %u OFFSET=%u \n", rtp->timestamp, recorder->paused_ts, recorder->offset_ts);
+		}
+		recorder->paused_ts = 0;


What happens here when you pause/resume more than once? Not sure this would still work. Besides, there's no fix to the sequence numbers, which means they won't be sequential, with big holes in between. This will cause severe issues when trying to replay them via Record&Play, because the recipient will think packets have been missed, and NACK them. This is one of the reasons why I'm insisting on using the RTP context here too, as it's one of the things it takes care of for you.

lminiero · 2018-02-26T11:12:32Z

Closing as this is based on a pull request we since closed (ICE restarts).

Added changes to pause & resume recorder

0550bf3

lminiero reviewed Jun 20, 2017

View reviewed changes

Added code review chnages for recording pause

23e6fae

lminiero closed this Feb 26, 2018

lminiero mentioned this pull request Jul 5, 2021

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added changes to pause & resume recorder #918

Added changes to pause & resume recorder #918

AjayChoudary commented Jun 4, 2017

lminiero commented Jun 5, 2017

AjayChoudary commented Jun 5, 2017

lminiero commented Jun 9, 2017

lminiero left a comment

lminiero Jun 20, 2017

lminiero Jun 20, 2017

lminiero Jun 20, 2017

lminiero Jun 20, 2017

lminiero Jun 20, 2017

lminiero Jun 20, 2017

lminiero Jun 20, 2017

lminiero Jun 20, 2017

lminiero Jun 20, 2017

lminiero Jun 20, 2017

lminiero commented Feb 26, 2018

Added changes to pause & resume recorder #918

Added changes to pause & resume recorder #918

Conversation

AjayChoudary commented Jun 4, 2017

lminiero commented Jun 5, 2017

AjayChoudary commented Jun 5, 2017

lminiero commented Jun 9, 2017

lminiero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lminiero commented Feb 26, 2018