Save media extmaps to recordings #2422

isnumanagic · 2020-11-04T06:32:53Z

This PR allows recordplay plugin and recorder to save and read media extmaps in .mjr header as array with x key. This in turn allows recordings with video-orient extmap to replay with correct orientation (among others), and allows rtp header extensions to be correctly mapped in janus-pp-rec tool without needing to pass --videoorient-ext or --audiolevel-ext params.

lminiero

While I applaud the effort (thanks!), I think there are several things done wrong here.

First of all, using a list to store these mappings is IMO incorrect: since you want to know which numeric ID corresponds to which extension, it should be a table instead (which would make it easier to save and process in mjr files too).
I don't like how the Record&Play plugin is pregenerating janus_sdp_attribute elements and storing them in a list, to append them brutally to the SDP. This will most likely cause issues when renegotiations are done, since it may result in those attributes being deallocated and still referenced by the list in the recording object. If all we keep is a map, we can regenerate the attributes any time we need them.
I added other notes inline on some of the things I noticed that are wrong or dangerous.

lminiero · 2020-11-04T09:27:27Z

plugins/janus_recordplay.c

+	GList *filtered = NULL;
+	while(attributes != NULL) {
+		janus_sdp_attribute *attr = (janus_sdp_attribute *)attributes->data;
+		if(!strcasecmp(attr->name, "extmap"))


Action is multiline, per code style there should be brackets (less ambiguous, more readable).

lminiero · 2020-11-04T09:28:42Z

plugins/janus_recordplay.c

@@ -562,16 +597,19 @@ static const char *janus_recordplay_parse_codec(const char *dir, const char *fil
 			offset += 2;
 			if(len > 0 && !parsed_header) {
 				/* This is the info header */
-				bytes = fread(prebuffer, sizeof(char), len, file);
+				char *json_header = malloc(sizeof(char)*(len+1));


Please use g_malloc/g_free. That said, not sure why you're allocating? 1500 bytes should be more than enough even if extensions are in the json.

In theory it's possible (even though it's highly unlikely) for JSON header to have more than 1500 chars if there are many extmaps with long URIs, so I added json_header buffer here just in case.

lminiero · 2020-11-04T09:31:04Z

plugins/janus_recordplay.c

+						char delim = ':', **ext_split = g_strsplit(ext_str, &delim, 2);
+						janus_sdp_attribute *ext = janus_sdp_attribute_create(
+							g_strdup(ext_split[0]),
+							ext_split[1] ? g_strdup(ext_split[1]) : NULL);


This is wrong. It's assuming g_strsplit will succeed, that there are at least two parts and that both are not NULL. This will segfault if any of those assumptions are incorrect. Besides, there's no need to g_strdup when doing janus_sdp_attribute_create, as that method already allocates new stuff for those strings: you're generating memory leaks, as those strings you're allocating are lost.

I've slightly misread g_strsplit documentation, thanks for pointing this out.

lminiero · 2020-11-04T09:32:08Z

plugins/janus_recordplay.c

@@ -653,15 +706,24 @@ static int janus_recordplay_generate_offer(janus_recordplay_recording *rec) {
 		JANUS_SDP_OA_AUDIO_PT, rec->audio_pt,
 		JANUS_SDP_OA_AUDIO_FMTP, rec->afmtp,
 		JANUS_SDP_OA_AUDIO_DIRECTION, JANUS_SDP_SENDONLY,
-		JANUS_SDP_OA_AUDIO_EXTENSION, JANUS_RTP_EXTMAP_MID, 1,


Do NOT remove this: mid is NOT one of the extensions you're notified about in the extmap. It's always added by the core, and so it must always be there.

lminiero · 2020-11-04T09:32:18Z

plugins/janus_recordplay.c

 		JANUS_SDP_OA_VIDEO, offer_video,
 		JANUS_SDP_OA_VIDEO_CODEC, janus_videocodec_name(rec->vcodec),
 		JANUS_SDP_OA_VIDEO_FMTP, rec->vfmtp,
 		JANUS_SDP_OA_VIDEO_PT, rec->video_pt,
 		JANUS_SDP_OA_VIDEO_DIRECTION, JANUS_SDP_SENDONLY,
-		JANUS_SDP_OA_VIDEO_EXTENSION, JANUS_RTP_EXTMAP_MID, 1,


lminiero · 2020-11-04T09:33:27Z

plugins/janus_recordplay.c

+	}
+	if(offer_video) {
+		janus_sdp_mline *video = janus_sdp_mline_find(offer, JANUS_SDP_VIDEO);
+		video->attributes = g_list_concat(video->attributes,


lminiero · 2020-11-04T09:35:17Z

plugins/janus_recordplay.c

+				}
+				GList *attr = janus_sdp_mline_find(answer, JANUS_SDP_AUDIO)->attributes;
+				rec->audio_extmaps = janus_recordplay_filter_extmaps(attr);
+				session->arc->extmaps = janus_recordplay_bake_extmaps(rec->audio_extmaps);


Not checking if session->arc exists.

lminiero · 2020-11-04T09:35:25Z

plugins/janus_recordplay.c

+				}
+				GList *attr = janus_sdp_mline_find(answer, JANUS_SDP_VIDEO)->attributes;
+				rec->video_extmaps = janus_recordplay_filter_extmaps(attr);
+				session->vrc->extmaps = janus_recordplay_bake_extmaps(rec->video_extmaps);


lminiero · 2020-11-04T09:38:20Z

record.h

@@ -48,6 +49,8 @@ typedef struct janus_recorder {
 	char *codec;
 	/*! \brief Codec-specific info (e.g., H.264 or VP9 profile) */
 	char *fmtp;
+	/*! \brief Media extmaps */
+	GList *extmaps;


I think using a list here is wrong. It should be a table, if the purpose is mapping a numeric extension ID to the extension namespace. Not even sure what the value saved there is?

lminiero · 2020-11-04T09:39:04Z

record.c

+				json_array_append_new(ext_json, json_string(ext->data));
+				ext = ext->next;
+			}
+			json_object_set(info, "x", ext_json);


If the purpose is having mappings, here, this should be a key/value object, not a serialized string.

isnumanagic · 2020-11-04T09:52:31Z

Thanks for the quick review, I'll try to update the PR to address the comments sometime today.

isnumanagic · 2020-11-04T22:41:14Z

Changed extmaps to use GHashMap * mappings (with key char *id and value char *info types, where info is extmap value after id). JSON .mjr header also updated to keeps object instead of array for extmap x key.

postprocessing/janus-pp-rec.c

postprocessing/mjr2pcap.c

isnumanagic · 2020-11-16T14:07:02Z

Hi @lminiero @atoppi. Could I request review for updated PR?

lminiero · 2020-11-16T14:36:17Z

Sorry, we're busy with the IETF meeting, so we won't be able to do much before next week.

lminiero

Added some more notes. Thanks!

lminiero · 2020-11-23T14:51:15Z

plugins/janus_recordplay.c

+	g_hash_table_remove_all(extmaps);
+	for(; attributes != NULL; attributes = attributes->next) {
+		janus_sdp_attribute *attr = (janus_sdp_attribute *)attributes->data;
+		if(!strcasecmp(attr->name, "extmap")) {


This seems to expect both attr->name and attr->value to be valid and not NULL: there should be a check on both.

lminiero · 2020-11-23T14:59:46Z

record.c

+			GHashTableIter iter;
+			char *ext_id, *ext_info;
+			g_hash_table_iter_init(&iter, recorder->extmaps);
+			while(g_hash_table_iter_next(&iter, (gpointer *)&ext_id, (gpointer *)&ext_info))


Why the & before the variables? They're pointers already.

As a side note, this isn't checking whether either ext_id or ext_info are NULL.

Because values saved within the table are of type char *, so i pass pointer to value type char * so it can be populated while iterating.

lminiero · 2020-11-23T15:01:29Z

postprocessing/janus-pp-rec.c

+		if(audio_level_extmap_id < 1 && strstr(ext[i], JANUS_RTP_EXTMAP_AUDIO_LEVEL))
+			audio_level_extmap_id = atoi(ext[i]);
+		if(video_orient_extmap_id < 1 && strstr(ext[i], JANUS_RTP_EXTMAP_VIDEO_ORIENTATION))
+			video_orient_extmap_id = atoi(ext[i]);


This isn't checking if ext[i] is NULL.

lminiero · 2020-11-23T15:03:02Z

postprocessing/janus-pp-rec.c

+						exit(1);
+					}
+					ext_n = json_object_size(ext_json);
+					ext = g_new(char *, ext_n);


What if this is an empty object? ext_n will be 0.

lminiero · 2020-11-23T15:06:23Z

plugins/janus_recordplay.c

+				if(ext_json && json_is_object(ext_json)) {
+					g_hash_table_remove_all(extmaps);
+					json_object_foreach(ext_json, ext_id, ext_info)
+						g_hash_table_insert(extmaps, g_strdup(ext_id), (gpointer)json_string_value(ext_info));


See above: not checking if those strings are NULL.

lminiero · 2020-11-23T15:07:12Z

plugins/janus_recordplay.c

+	g_hash_table_iter_init(&iter, extmaps);
+	while(g_hash_table_iter_next(&iter, (gpointer *)&ext_id, (gpointer *)&ext_info))
+		g_hash_table_insert(mapped, g_strdup(ext_id), g_strdup(ext_info));
+	/* Remove existing extmaps */


I think this is quite wrong: generate offer adds a mid extmap, that you'd be removing here. The only thing that you should be doing here is possibly see if an attribute you're adding now already exists (maybe with a different ID), or if there are ID conflicts, so those are the only ones you should remove IMO.

Generated extmaps are merged with media attributes into temporary merged hashmap, with former having priority, so this would not remove mid extmap as it would be present in merged hashmap and re-added further down. However, upon inspecting this part of code, it is not very clear how it does what it does, and has some possible edge case errors, so i will rewrite it.

isnumanagic · 2020-11-24T10:37:28Z

Thanks for the review @lminiero. Sorry for pinging you while you were attending IETF meeting, hope it went well though! I have updated the PR per your comments, and rebased it to master to check for possible conflicts.

isnumanagic · 2020-11-24T10:52:57Z

sdp-utils.c

@@ -1282,7 +1282,7 @@ janus_sdp *janus_sdp_generate_offer(const char *name, const char *address, ...)
 				char *extmap = g_hash_table_lookup(audio_extids, iter->data);
 				if(extmap != NULL) {
 					janus_sdp_attribute *a = janus_sdp_attribute_create("extmap",
-						"%d %s\r\n", GPOINTER_TO_INT(iter->data), extmap);


Correct me if newlines were supposed to be here, but they do not appear in any other janus_sdp_attribute_create calls. I removed them because they mess up extmap value matching (I could've just trimmed extmap values when processing but this seemed to be more correct).

atoppi · 2020-11-26T14:37:38Z

plugins/janus_recordplay.c

+			JANUS_LOG(LOG_WARN, "Extmap has null fields\n");
+			continue;
+		}
+		JANUS_LOG(LOG_INFO, " > %d:%s\n", *ext_id, ext_info);


I'd make this log more specific than just id > info.
Also consider lowering its level to verbose.

This is one of debug logs I used for testing, it shouldn't be here. Thanks for spotting this.

lminiero · 2020-11-30T11:07:11Z

To be honest after discussing with @atoppi this still feels very convoluted for us. It looks like everything is handled in a more complex way than it should. From what we gather, this basically needs these core functionality:

a hashmap in record.h to track extensions in recordings
a way to add/remove extensions to a new mjr (the same way we can mark a recording as e2ee, for instance)
a way for interested plugins (e.g., Record&Play) to use that info

Point 1. is indeed there, but 2. seems implicit (plugins work on the hashtable directly) which can be problematic. Point 3. is the part where I think it has been implemented in a way that is way more complex than necessary, with duplicated tables, stuff that is regenerated, etc., all things that will add a maintainance burden and will prevent an easy integration in other plugins in the future (just imagine doing all this in the VideoRoom!). It feels like all the plugin needs is a simple way to add extensions one by one when a recording is being created, and iterating on those from existing recordings to create the attributes the first time, without complex structures to back the functionality.

I understand we've asked for way too many changes already, so I don't expect you to completely refactor the effort now. I'll try to come up with a simpler way to address this requirement and publish a new PR (possibly by the end of the week), so that we can check if it solves your need in a cleaner way. We can keep this PR open for the time being until that happens.

isnumanagic · 2020-11-30T12:47:29Z

Thanks for the update. I agree with what has been said, especially with the third point - the current way of adding extmaps to SDP offer is not very elegant. I look forward to your PR.

isnumanagic · 2021-01-20T09:46:57Z

Hi @lminiero & @atoppi, just wanted to check on the status here. Is this in progress? If not, I can take over the refactor, but will need some pointers on how to do it best, i.e. what you guys had in mind.

lminiero · 2021-01-20T09:49:06Z

@isnumanagic apologies for the lack of feedback. I've been swamped with work so I had no time to work on this. I'll try to come up with something in the next few days or next week.

lminiero · 2021-01-22T16:53:15Z

@isnumanagic please see the PR mentioned above. I'll close this one in the meanwhile. Thanks again!

lminiero reviewed Nov 4, 2020

View reviewed changes

isnumanagic requested a review from lminiero November 5, 2020 12:33

atoppi reviewed Nov 9, 2020

View reviewed changes

postprocessing/janus-pp-rec.c Show resolved Hide resolved

postprocessing/mjr2pcap.c Show resolved Hide resolved

lminiero reviewed Nov 23, 2020

View reviewed changes

Ishak Numanagic added 7 commits November 24, 2020 11:35

Save media extmaps to recordings

0de9a18

Small cleanup

f33c856

Adressing CR

a667910

Small fixes

13adc7d

Small fixes

eb2a314

Fix extmap reading in janus-pp-rec

9e48ee5

CR & small fixes

1770a8f

isnumanagic force-pushed the recordplay-media-extmaps branch from 0f0860e to 1770a8f Compare November 24, 2020 10:35

isnumanagic commented Nov 24, 2020

View reviewed changes

Change hashmap key type to int*; Update attribute regen logic

8425df2

isnumanagic requested review from lminiero and atoppi November 26, 2020 10:57

atoppi reviewed Nov 26, 2020

View reviewed changes

Remove redundant log

87b5027

lminiero mentioned this pull request Jan 22, 2021

Allow marking of RTP extensions in MJR recordings #2527

Merged

lminiero closed this Jan 22, 2021

isnumanagic deleted the recordplay-media-extmaps branch June 29, 2021 12:13

Save media extmaps to recordings #2422

Save media extmaps to recordings #2422

Conversation

isnumanagic commented Nov 4, 2020 • edited Loading

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

isnumanagic Nov 4, 2020 • edited Loading

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

isnumanagic commented Nov 4, 2020

isnumanagic commented Nov 4, 2020

isnumanagic commented Nov 16, 2020

lminiero commented Nov 16, 2020

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

isnumanagic Nov 24, 2020 • edited Loading

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

isnumanagic commented Nov 24, 2020

isnumanagic Nov 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lminiero commented Nov 30, 2020

isnumanagic commented Nov 30, 2020

isnumanagic commented Jan 20, 2021

lminiero commented Jan 20, 2021

lminiero commented Jan 22, 2021

isnumanagic commented Nov 4, 2020 •

edited

Loading

isnumanagic Nov 4, 2020 •

edited

Loading

isnumanagic Nov 24, 2020 •

edited

Loading

isnumanagic Nov 24, 2020 •

edited

Loading