Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added changes to pause & resume recorder #918

Closed
wants to merge 2 commits into from
Closed

Added changes to pause & resume recorder #918

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0550bf3
Added changes to pause & resume recorder
AjayChoudary Jun 4, 2017
23e6fae
Added code review chnages for recording pause
AjayChoudary Jun 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 54 additions & 3 deletions plugins/janus_videoroom.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

gchar *recording_base; /* Base name for the recording (e.g., /path/to/filename, will generate /path/to/filename-audio.mjr and/or /path/to/filename-video.mjr */
janus_recorder *arc; /* The Janus recorder instance for this publisher's audio, if enabled */
janus_recorder *vrc; /* The Janus recorder instance for this publisher's video, if enabled */
Expand Down Expand Up @@ -602,7 +603,6 @@ static void janus_videoroom_publisher_free(const janus_refcount *p_ref) {
static void janus_videoroom_session_dereference(janus_videoroom_session *session) {
janus_refcount_decrease(&session->ref);
}

static void janus_videoroom_session_destroy(janus_videoroom_session *session) {
if(session && g_atomic_int_compare_and_exchange(&session->destroyed, 0, 1))
janus_refcount_decrease(&session->ref);
Expand Down Expand Up @@ -2222,7 +2222,7 @@ struct janus_plugin_result *janus_videoroom_handle_message(janus_plugin_session
} else if(!strcasecmp(request_text, "join") || !strcasecmp(request_text, "joinandconfigure")
|| !strcasecmp(request_text, "configure") || !strcasecmp(request_text, "publish") || !strcasecmp(request_text, "unpublish")
|| !strcasecmp(request_text, "start") || !strcasecmp(request_text, "pause") || !strcasecmp(request_text, "switch")
|| !strcasecmp(request_text, "stop") || !strcasecmp(request_text, "leave")) {
|| !strcasecmp(request_text, "stop") || !strcasecmp(request_text, "keyframe") || !strcasecmp(request_text, "leave")) {
/* These messages are handled asynchronously */

janus_videoroom_message *msg = g_malloc0(sizeof(janus_videoroom_message));
Expand Down Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if(participant->arc) {
janus_recorder_pause(participant->arc, paused);
}
if(participant->vrc) {
JANUS_LOG(LOG_INFO, "Pause:%d video recording %s\n", paused, participant->vrc->filename ? participant->vrc->filename : "");
janus_recorder_pause(participant->vrc, paused);
}
if(participant->drc) {
janus_recorder_pause(participant->drc, paused);
}
}
static void janus_videoroom_recorder_close(janus_videoroom_publisher *participant) {
if(participant->arc) {
janus_recorder_close(participant->arc);
Expand Down Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

publisher->recording_base = NULL;
publisher->arc = NULL;
publisher->vrc = NULL;
Expand Down Expand Up @@ -3259,6 +3271,7 @@ static void *janus_videoroom_handler(void *data) {
json_t *data = json_object_get(root, "data");
json_t *bitrate = json_object_get(root, "bitrate");
json_t *record = json_object_get(root, "record");
json_t *record_pause = json_object_get(root, "record_pause");
json_t *recfile = json_object_get(root, "filename");
json_t *display = json_object_get(root, "display");
json_t *refresh = json_object_get(root, "refresh");
Expand Down Expand Up @@ -3349,6 +3362,28 @@ static void *janus_videoroom_handler(void *data) {
}
}
}
if(record_pause) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if(participant->recording_pause != json_is_true(record_pause)) {
JANUS_LOG(LOG_VERB, "Recording video, PAUSE changed %d to %d \n", participant->recording_pause, json_is_true(record_pause));
participant->recording_pause = json_is_true(record_pause);
janus_videoroom_recorder_pause(participant, participant->recording_pause);
if(!participant->recording_pause && strstr(participant->sdp, "m=video")) {
/* Send a FIR */
char buf[20];
memset(buf, 0, 20);
janus_rtcp_fir((char *)&buf, 20, &participant->fir_seq);
JANUS_LOG(LOG_VERB, "Recording RESUME video, sending FIR to %"SCNu64" (%s)\n",
participant->user_id, participant->display ? participant->display : "??");
gateway->relay_rtcp(participant->session->handle, 1, buf, 20);
/* Send a PLI too, just in case... */
memset(buf, 0, 12);
janus_rtcp_pli((char *)&buf, 12);
JANUS_LOG(LOG_VERB, "Recording RESUME video, sending PLI to %"SCNu64" (%s)\n",
participant->user_id, participant->display ? participant->display : "??");
gateway->relay_rtcp(participant->session->handle, 1, buf, 12);
}
}
}
janus_mutex_unlock(&participant->rec_mutex);
if(display) {
janus_mutex_lock(&participant->room->mutex);
Expand Down Expand Up @@ -3398,6 +3433,22 @@ static void *janus_videoroom_handler(void *data) {
}
gateway->notify_event(&janus_videoroom_plugin, session->handle, info);
}
} else if(!strcasecmp(request_text, "keyframe")) {
if(strstr(participant->sdp, "m=video")) {
/* Send a FIR */
char buf[20];
memset(buf, 0, 20);
janus_rtcp_fir((char *)&buf, 20, &participant->fir_seq);
JANUS_LOG(LOG_VERB, "Video keyframe , sending FIR to %"SCNu64" (%s)\n",
participant->user_id, participant->display ? participant->display : "??");
gateway->relay_rtcp(participant->session->handle, 1, buf, 20);
/* Send a PLI too, just in case... */
memset(buf, 0, 12);
janus_rtcp_pli((char *)&buf, 12);
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);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} else if(!strcasecmp(request_text, "unpublish")) {
/* This participant wants to unpublish */
if(!participant->sdp) {
Expand Down
50 changes: 47 additions & 3 deletions record.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include "record.h"
#include "debug.h"
#include "utils.h"
#include "rtp.h"

#define htonll(x) ((1==htonl(1)) ? (x) : ((gint64)htonl((x) & 0xFFFFFFFF) << 32) | htonl((x) >> 32))
#define ntohll(x) ((1==ntohl(1)) ? (x) : ((gint64)ntohl((x) & 0xFFFFFFFF) << 32) | ntohl((x) >> 32))
Expand Down Expand Up @@ -92,6 +93,9 @@ janus_recorder *janus_recorder_create(const char *dir, const char *codec, const
rc->file = NULL;
rc->codec = g_strdup(codec);
rc->created = janus_get_real_time();
rc->pause = 0;
rc->paused_ts = 0;
rc->offset_ts = 0;
if(dir != NULL) {
/* Check if this directory exists, and create it if needed */
struct stat s;
Expand Down Expand Up @@ -166,6 +170,7 @@ janus_recorder *janus_recorder_create(const char *dir, const char *codec, const
}

int janus_recorder_save_frame(janus_recorder *recorder, char *buffer, uint length) {
rtp_header *rtp = (rtp_header *)buffer;
if(!recorder)
return -1;
janus_mutex_lock_nodebug(&recorder->mutex);
Expand Down Expand Up @@ -205,17 +210,43 @@ int janus_recorder_save_frame(janus_recorder *recorder, char *buffer, uint lengt
/* Done */
recorder->header = 1;
}
if(recorder->pause) {
if(!recorder->paused_ts) {
if(recorder->type == JANUS_RECORDER_DATA) {
recorder->paused_ts = janus_get_real_time();
} else {
recorder->paused_ts = ntohl(rtp->timestamp);
}
JANUS_LOG(LOG_INFO, "PAUSED TIME TS = %u, OFFSET=%u \n", recorder->paused_ts, recorder->offset_ts);
}
janus_mutex_unlock_nodebug(&recorder->mutex);
return 0;
} else if(recorder->paused_ts) {
if(recorder->type == JANUS_RECORDER_DATA) {
recorder->offset_ts += janus_get_real_time() - recorder->paused_ts;
} else {
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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
/* Write frame header */
fwrite(frame_header, sizeof(char), strlen(frame_header), recorder->file);
uint16_t header_bytes = htons(recorder->type == JANUS_RECORDER_DATA ? (length+sizeof(gint64)) : length);
fwrite(&header_bytes, sizeof(uint16_t), 1, recorder->file);
/* Save packet on file */
int temp = 0, tot = length;
if(recorder->type == JANUS_RECORDER_DATA) {
/* If it's data, then we need to prepend timing related info, as it's not there by itself */
gint64 now = htonll(janus_get_real_time());
gint64 now = htonll(janus_get_real_time() - recorder->offset_ts);
fwrite(&now, sizeof(gint64), 1, recorder->file);
} else {
uint32_t ts = rtp->timestamp;
rtp->timestamp = htonl( ntohl(rtp->timestamp) - recorder->offset_ts);
fwrite(buffer, sizeof(char), 12, recorder->file);
tot -= 12;
rtp->timestamp = ts;
}
/* Save packet on file */
int temp = 0, tot = length;
while(tot > 0) {
temp = fwrite(buffer+length-tot, sizeof(char), tot, recorder->file);
if(temp <= 0) {
Expand All @@ -230,6 +261,19 @@ int janus_recorder_save_frame(janus_recorder *recorder, char *buffer, uint lengt
return 0;
}

int janus_recorder_pause(janus_recorder *recorder, gboolean paused) {
if(!recorder || !recorder->writable)
return -1;
janus_mutex_lock_nodebug(&recorder->mutex);
/*
fwrite(pause_header, sizeof(char), strlen(frame_header), recorder->file);
uint16_t header_bytes = htons(0);
fwrite(&header_bytes, sizeof(uint16_t), 1, recorder->file);
*/
recorder->pause = paused ? 1:0;
janus_mutex_unlock_nodebug(&recorder->mutex);
return 0;
}
int janus_recorder_close(janus_recorder *recorder) {
if(!recorder || !recorder->writable)
return -1;
Expand Down
9 changes: 9 additions & 0 deletions record.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pausing and resuming

gint64 paused_ts, offset_ts;
/*! \brief Whether recording paused or not */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whether the recording has been paused or not

int pause:1;
/*! \brief Whether the info header for this recorder instance has already been written or not */
int header:1;
/*! \brief Whether this recorder instance can be used for writing or not */
Expand Down Expand Up @@ -77,6 +81,11 @@ janus_recorder *janus_recorder_create(const char *dir, const char *codec, const
* @param[in] length The frame data length
* @returns 0 in case of success, a negative integer otherwise */
int janus_recorder_save_frame(janus_recorder *recorder, char *buffer, uint length);
/*! \brief Pause/Resume the recorder
* @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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/*! \brief Close the recorder
* @param[in] recorder The janus_recorder instance to close
* @returns 0 in case of success, a negative integer otherwise */
Expand Down