Add quirk for RTSP servers #2909

jp-bennett · 2022-03-01T18:30:17Z

Some RTSP Servers expect only the path, not the full url, for DESCRIBE, SETUP, and PLAY commands.

januscla · 2022-03-01T18:30:19Z

Thanks for your contribution, @jp-bennett! Please make sure you sign our CLA, as it's a required step before we can merge this.

lminiero

Thanks for the patch! I added a few notes below, mostly editorial for now, since the feature itself seems mostly straightforward.

lminiero · 2022-03-01T18:33:50Z

plugins/janus_streaming.c

@@ -1096,6 +1096,7 @@ typedef struct janus_streaming_rtp_source {
 	janus_streaming_buffer *curldata;
 	char *rtsp_url;
 	char *rtsp_username, *rtsp_password;
+        char *rtsp_stream_uri;


Code style: please use tabs and not spaces for indentation.

Lots of little code style things -- I blame muscle memory. I'll fix these up, and add the config option, hopefully tonight.

lminiero · 2022-03-01T18:34:44Z

plugins/janus_streaming.c

@@ -6519,6 +6521,23 @@ static int janus_streaming_rtsp_connect_to_server(janus_streaming_mountpoint *mp
 	}
 	long code = 0;
 	res = curl_easy_getinfo(curl, CURLINFO_RESPONSE_CODE, &code);
+#if CURL_AT_LEAST_VERSION(7, 62, 0)
+	if(code == 404) { //Possibly a quirk in the RTSP server, where the DESCRIBE request expects a path only.


Just a nit, per code style we only use /* */ for comments: I'd also put it as the first line inside the if, rather than on the same line as the if itself (just for the sake of readability).

lminiero · 2022-03-01T18:36:08Z

plugins/janus_streaming.c

+	if(code == 404) { //Possibly a quirk in the RTSP server, where the DESCRIBE request expects a path only.
+		CURLU *curl_u;
+		char *path;
+		curl_u = curl_url();


I'd collapse the definition as CURLU *curl_u = curl_url();, no need for pre-definitions. Besides, path should be initialized to NULL, since otherwise it may point to random garbage and the &path may be dangerous.

lminiero · 2022-03-01T18:38:24Z

plugins/janus_streaming.c

+			if(!(curl_url_get(curl_u, CURLUPART_PATH, &path, 0))) {
+				curl_easy_setopt(curl, CURLOPT_RTSP_STREAM_URI, path);
+				res = curl_easy_perform(curl);
+                                source->rtsp_stream_uri = path;


Indentation broken here (spaces instead of tabs). That said, I'd do a g_strdup() for rtsp_stream_uri, so you can use the curl_free(path) you commented below.

lminiero · 2022-03-01T18:39:53Z

plugins/janus_streaming.c

@@ -5795,6 +5796,7 @@ static void janus_streaming_rtp_source_free(janus_streaming_rtp_source *source)
 	g_free(source->rtsp_url);
 	g_free(source->rtsp_username);
 	g_free(source->rtsp_password);
+	curl_free(source->rtsp_stream_uri);


If we use g_strdup, this should be g_free, which will make it consistent with all other strings we keep track of.

lminiero · 2022-03-01T18:40:43Z

plugins/janus_streaming.c

+	if(source->rtsp_stream_uri)
+		curl_easy_setopt(source->curl, CURLOPT_RTSP_STREAM_URI, source->rtsp_stream_uri);
+	else
+		curl_easy_setopt(source->curl, CURLOPT_RTSP_STREAM_URI, source->rtsp_url);


This might be simplified with a single line using a ternary operator:

curl_easy_setopt(source->curl, CURLOPT_RTSP_STREAM_URI, source->rtsp_stream_uri ? source->rtsp_stream_uri : source->rtsp_url);

lminiero · 2022-03-01T18:53:08Z

I just noticed this is a PR for 0.x, not master. I'll port the changes to master as well if this is merged, since the RTSP code has seen little change, and so would apply there too.

jp-bennett · 2022-03-02T04:39:15Z

@lminiero The second and third commits are compile tested only. I've temporarily lost access to the camera that demonstrated this quirk. Let me know if you see anything obviously wrong, and I'll test this in the real world once I'm able.

lminiero

Thanks for the quick changes, but I do see some problems in the code.

lminiero · 2022-03-02T09:35:53Z

plugins/janus_streaming.c

@@ -144,6 +144,8 @@ The following options are only valid for the 'rtsp' type:
 url = RTSP stream URL
 rtsp_user = RTSP authorization username, if needed
 rtsp_pwd = RTSP authorization password, if needed
+rtsp_quirk = Some RTSP servers offer the stream using only the path, instead of the fully qualified URL.
+	If set true, this boolean informs Janus that we should try a path-only DESCRIBE request if the initial request returns 404.


These two lines should be added to janus.plugin.streaming.jcfg.in as well.

lminiero · 2022-03-02T09:36:24Z

plugins/janus_streaming.c

@@ -2132,6 +2136,7 @@ int janus_streaming_init(janus_callbacks *callback, const char *config_path) {
 				janus_config_item *file = janus_config_get(config, cat, janus_config_type_item, "url");
 				janus_config_item *username = janus_config_get(config, cat, janus_config_type_item, "rtsp_user");
 				janus_config_item *password = janus_config_get(config, cat, janus_config_type_item, "rtsp_pwd");
+				janus_config_item *quirk = janus_config_get(config, cat, janus_config_type_item, "quirk");


Isn't the property called rtsp_quirk?

lminiero · 2022-03-02T09:37:40Z

plugins/janus_streaming.c

@@ -2155,6 +2160,7 @@ int janus_streaming_init(janus_callbacks *callback, const char *config_path) {
 					continue;
 				}
 				gboolean is_private = priv && priv->value && janus_is_true(priv->value);
+				gboolean rtsp_quirk = quirk ? TRUE : FALSE;


This should be

gboolean rtsp_quirk = quirk && quirk->value && janus_is_true(quirk->value);

The way you added the ternary operator now, the property will be set to TRUE any time it is available in the config file, even if it's actually set to no or false.

lminiero · 2022-03-02T09:39:36Z

plugins/janus_streaming.c

@@ -3264,6 +3274,7 @@ static json_t *janus_streaming_process_synchronous_request(janus_streaming_sessi
 					(char *)json_string_value(url),
 					username ? (char *)json_string_value(username) : NULL,
 					password ? (char *)json_string_value(password) : NULL,
+					quirk ? TRUE : FALSE,


Same mistake as above. If you check how we deal with boolean properties in JSON requests, we do something like this:

gboolean dovideo = video ? json_is_true(video) : FALSE;

so you should do something similar with your new feature, and pass the boolean to janus_streaming_create_rtsp_source.

lminiero · 2022-03-02T09:40:18Z

plugins/janus_streaming.c

@@ -3417,6 +3428,7 @@ static json_t *janus_streaming_process_synchronous_request(janus_streaming_sessi
 					janus_config_add(config, c, janus_config_item_create("rtsp_user", source->rtsp_username));
 				if(source->rtsp_password)
 					janus_config_add(config, c, janus_config_item_create("rtsp_pwd", source->rtsp_password));
+				janus_config_add(config, c, janus_config_item_create("rtsp_quirk", source->rtsp_quirk ? "yes" : "no"));


I'd actually only save the permanent feature if rtsp_quirk is true, no need to save it if it isn't.

lminiero · 2022-03-02T09:40:25Z

plugins/janus_streaming.c

@@ -3631,6 +3643,7 @@ static json_t *janus_streaming_process_synchronous_request(janus_streaming_sessi
 						janus_config_add(config, c, janus_config_item_create("rtsp_user", source->rtsp_username));
 					if(source->rtsp_password)
 						janus_config_add(config, c, janus_config_item_create("rtsp_pwd", source->rtsp_password));
+					janus_config_add(config, c, janus_config_item_create("rtsp_quirk", source->rtsp_quirk ? "yes" : "no"));


lminiero · 2022-03-02T09:40:56Z

plugins/janus_streaming.c

@@ -6519,6 +6533,23 @@ static int janus_streaming_rtsp_connect_to_server(janus_streaming_mountpoint *mp
 	}
 	long code = 0;
 	res = curl_easy_getinfo(curl, CURLINFO_RESPONSE_CODE, &code);
+#if CURL_AT_LEAST_VERSION(7, 62, 0)
+	if(source->rtsp_quirk && code == 404) {
+		/*Possibly a quirk in the RTSP server, where the DESCRIBE request expects a path only.*/


Small nit, you should leave a space after /* and before */ (I know, I'm annoying 🤣 )

lminiero · 2022-03-02T09:42:40Z

plugins/janus_streaming.c

+			if(!(curl_url_get(curl_u, CURLUPART_PATH, &path, 0))) {
+				curl_easy_setopt(curl, CURLOPT_RTSP_STREAM_URI, path);
+				res = curl_easy_perform(curl);
+				source->rtsp_stream_uri = g_strdup(path);


Shouldn't we check what curl_easy_perform and/or curl_easy_getinfo returned, before saving this property? If we got a 404 or a different error again, then the quirk didn't help. Besides, path may still be NULL maybe?

I don't think path can be null. The curl documentation seems to indicate that it will be set to "/" at the worst. (https://curl.se/libcurl/c/curl_url_get.html)

lminiero · 2022-03-02T09:43:52Z

plugins/janus_streaming.c

@@ -7166,6 +7197,8 @@ janus_streaming_mountpoint *janus_streaming_create_rtsp_source(
 	live_rtsp_source->rtsp_url = g_strdup(url);
 	live_rtsp_source->rtsp_username = username ? g_strdup(username) : NULL;
 	live_rtsp_source->rtsp_password = password ? g_strdup(password) : NULL;
+	live_rtsp_source->rtsp_stream_uri = NULL;
+	live_rtsp_source->rtsp_quirk = FALSE;


Shouldn't this be equal to the quirk that's been passed to janus_streaming_create_rtsp_source?

Some RTSP Servers expect only the path, not the full url, for DESCRIBE, SETUP, and PLAY commands.

jp-bennett · 2022-03-02T16:40:48Z

@lminiero Next revision, with your comments addressed. Went ahead and squashed to a single commit

lminiero

Thanks for the fixes! Noticed just a couple things left to fix 👍

lminiero · 2022-03-02T16:40:58Z

plugins/janus_streaming.c

@@ -2155,6 +2160,7 @@ int janus_streaming_init(janus_callbacks *callback, const char *config_path) {
 					continue;
 				}
 				gboolean is_private = priv && priv->value && janus_is_true(priv->value);
+				gboolean rtsp_quirk = quirk && quirk->value && janus_is_true(priv->value);


This should be janus_is_true(quirk->value) (my typo in the original message, sorry)

This feels less like black magic, with the correct value in there. =)

lminiero · 2022-03-02T16:42:24Z

plugins/janus_streaming.c

+				res = curl_easy_getinfo(curl, CURLINFO_RESPONSE_CODE, &code);
+				if((res == CURLE_OK) && (code != 404)) {
+					source->rtsp_stream_uri = g_strdup(path);
+					curl_free(path);


The curl_free should be outside of the if, as otherwise the path string will leak if an error occurred.

lminiero · 2022-03-02T16:43:44Z

Went ahead and squashed to a single commit

Thanks but no need to do that, as I squash when I merge anyway.

lminiero · 2022-03-02T16:51:14Z

Thanks! This looks good to me now. If you can make a test with those quirky cameras and confirm it works as expected for you, this is good to merge for me. Before we can do that, though, we'd need you to sign the CLA.

lminiero · 2022-03-02T17:05:54Z

plugins/janus_streaming.c

@@ -3417,6 +3429,8 @@ static json_t *janus_streaming_process_synchronous_request(janus_streaming_sessi
 					janus_config_add(config, c, janus_config_item_create("rtsp_user", source->rtsp_username));
 				if(source->rtsp_password)
 					janus_config_add(config, c, janus_config_item_create("rtsp_pwd", source->rtsp_password));
+				if(source->rtsp_password)


Just noticed a typo: this should be if(source->rtsp_quirk).

lminiero · 2022-03-02T17:06:04Z

plugins/janus_streaming.c

@@ -3631,6 +3645,8 @@ static json_t *janus_streaming_process_synchronous_request(janus_streaming_sessi
 						janus_config_add(config, c, janus_config_item_create("rtsp_user", source->rtsp_username));
 					if(source->rtsp_password)
 						janus_config_add(config, c, janus_config_item_create("rtsp_pwd", source->rtsp_password));
+					if(source->rtsp_password)


lminiero · 2022-03-03T10:32:50Z

I'll merge and fix those two small typos myself. Thanks again for your contribution!

…nly DESCRIBE requests (see #2909)

lminiero · 2022-03-03T10:35:37Z

Fixed the typo in 0.x, and ported the changes to master as well.

This PR contains the following updates: | Package | Update | Change | |---|---|---| | [meetecho/janus-gateway](https://github.com/meetecho/janus-gateway) | major | `v0.11.7` -> `v1.0.0` | --- ### Release Notes <details> <summary>meetecho/janus-gateway</summary> ### [`v1.0.0`](https://github.com/meetecho/janus-gateway/blob/HEAD/CHANGELOG.md#v100---2022-03-03) [Compare Source](meetecho/janus-gateway@v0.12.0...v1.0.0) - Refactored Janus to support multistream PeerConnections \[[PR-2211](meetecho/janus-gateway#2211)] - Moved all source files under new 'src' folder to unclutter the repo \[[PR-2885](meetecho/janus-gateway#2885)] - Fixed definition of trylock wrapper when using pthreads \[[Issue-2894](meetecho/janus-gateway#2894)] - Fixed broken RTP when no extensions are negotiated - Added checks when inserting RTP extensions to avoid buffer overflows - Added missing support for disabled rid simulcast substreams in SDP \[[PR-2888](meetecho/janus-gateway#2888)] - Fixed TWCC feedback when simulcast SSRCs are missing (thanks [@OxleyS](https://github.com/OxleyS)!) \[[PR-2908](meetecho/janus-gateway#2908)] - Added support for playout-delay RTP extension \[[PR-2895](meetecho/janus-gateway#2895)] - Fixed partially broken H.264 support when using Firefox in VideoRoom - Fixed new VideoRoom rtp_forward API ignoring some properties - Fixed deadlock and segfault when stopping Streaming mountpoint recordings \[[Issue-2902](meetecho/janus-gateway#2902)] - Fixed RTSP support in Streaming plugin for cameras that expect path-only DESCRIBE requests (thanks [@jp-bennett](https://github.com/jp-bennett)!) \[[PR-2909](meetecho/janus-gateway#2909)] - Fixed RTP being relayed incorrectly in Lua and Duktape plugins - Added Duktape as optional dependency, instead of embedding the engine code \[[PR-2886](meetecho/janus-gateway#2886)] - Fixed crash at startup when not able to connect to RabbitMQ server - Improved fuzzing and checks on RTP extensions - Removed distinction between simulcast and simulcast2 in janus.js \[[PR-2887](meetecho/janus-gateway#2887)] - Other smaller fixes and improvements (thanks to all who contributed pull requests and reported issues!) ### [`v0.12.0`](meetecho/janus-gateway@v0.11.8...v0.12.0) [Compare Source](meetecho/janus-gateway@v0.11.8...v0.12.0) ### [`v0.11.8`](https://github.com/meetecho/janus-gateway/blob/HEAD/CHANGELOG.md#v0118---2022-02-11) [Compare Source](meetecho/janus-gateway@v0.11.7...v0.11.8) - Added initial (and limited) integration of RED audio \[[PR-2685](meetecho/janus-gateway#2685)] - Added support for Two-Byte header RTP extensions (RFC8285) and, partially, for the new Depencency Descriptor RTP extension (needed for AV1-SVC) \[[PR-2741](meetecho/janus-gateway#2741)] - Fixed rare race conditions between sending a packet and closing a connection \[[PR-2869](meetecho/janus-gateway#2869)] - Fix last stats before closing PeerConnection not being sent to handlers (thanks [@zodiak83](https://github.com/zodiak83)!) \[[PR-2874](meetecho/janus-gateway#2874)] - Changed automatic allocation on static loops from round robin to least used \[[PR-2878](meetecho/janus-gateway#2878)] - Added new API to bulk start/stop MJR-based recordings in AudioBridge \[[PR-2862](meetecho/janus-gateway#2862)] - Fixed broken duration in spatial AudioBridge recordings - Fixed broken G.711 RTP forwarding in AudioBridge (thanks [@AlexYaremchuk](https://github.com/AlexYaremchuk)!) \[[PR-2875](meetecho/janus-gateway#2875)] - Fixed broken recordings in NoSIP plugin - Fixed warnings when postprocessing Opus recordings with DTX packets - Other smaller fixes and improvements (thanks to all who contributed pull requests and reported issues!) </details> --- ### Configuration 📅 **Schedule**: At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). Reviewed-on: https://git.walbeck.it/walbeck-it/docker-janus-gateway/pulls/65 Co-authored-by: renovate-bot <[email protected]> Co-committed-by: renovate-bot <[email protected]>

jp-bennett mentioned this pull request Mar 1, 2022

[0.x], [1.x] RTSP streaming fails to get DESCRIBE with 404 #2906

Closed

lminiero reviewed Mar 1, 2022

View reviewed changes

lminiero added the legacy Related to Janus 0.x label Mar 1, 2022

jp-bennett force-pushed the 0.x branch from 8807e39 to 18c526a Compare March 2, 2022 04:24

lminiero reviewed Mar 2, 2022

View reviewed changes

Add quirk for RTSP servers

1109e0b

Some RTSP Servers expect only the path, not the full url, for DESCRIBE, SETUP, and PLAY commands.

jp-bennett force-pushed the 0.x branch from 18c526a to 1109e0b Compare March 2, 2022 16:37

lminiero reviewed Mar 2, 2022

View reviewed changes

Fix typo and memory leak in rtsp_quirk

3703459

lminiero reviewed Mar 2, 2022

View reviewed changes

lminiero merged commit 413f134 into meetecho:0.x Mar 3, 2022

lminiero added a commit that referenced this pull request Mar 3, 2022

Fixed typos (see #2909)

08ca816

lminiero added a commit that referenced this pull request Mar 3, 2022

Fixed RTSP support in Streaming plugin for cameras that expect path-o…

4f0e0ca

…nly DESCRIBE requests (see #2909)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add quirk for RTSP servers #2909

Add quirk for RTSP servers #2909

jp-bennett commented Mar 1, 2022

januscla commented Mar 1, 2022

lminiero left a comment

lminiero Mar 1, 2022

jp-bennett Mar 2, 2022

lminiero Mar 1, 2022

lminiero Mar 1, 2022

lminiero Mar 1, 2022

lminiero Mar 1, 2022

lminiero Mar 1, 2022

lminiero commented Mar 1, 2022

jp-bennett commented Mar 2, 2022

lminiero left a comment

lminiero Mar 2, 2022

lminiero Mar 2, 2022

lminiero Mar 2, 2022 •

edited

Loading

lminiero Mar 2, 2022

lminiero Mar 2, 2022 •

edited

Loading

lminiero Mar 2, 2022

lminiero Mar 2, 2022

lminiero Mar 2, 2022

jp-bennett Mar 2, 2022

lminiero Mar 2, 2022

jp-bennett commented Mar 2, 2022

lminiero left a comment

lminiero Mar 2, 2022

jp-bennett Mar 2, 2022

lminiero Mar 2, 2022

jp-bennett Mar 2, 2022

lminiero commented Mar 2, 2022

lminiero commented Mar 2, 2022

lminiero Mar 2, 2022 •

edited

Loading

lminiero Mar 2, 2022

lminiero commented Mar 3, 2022

lminiero commented Mar 3, 2022

Add quirk for RTSP servers #2909

Add quirk for RTSP servers #2909

Conversation

jp-bennett commented Mar 1, 2022

januscla commented Mar 1, 2022

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

lminiero commented Mar 1, 2022

jp-bennett commented Mar 2, 2022

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

lminiero Mar 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lminiero Mar 2, 2022 • 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

jp-bennett commented Mar 2, 2022

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

lminiero commented Mar 2, 2022

lminiero commented Mar 2, 2022

lminiero Mar 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lminiero commented Mar 3, 2022

lminiero commented Mar 3, 2022

lminiero Mar 2, 2022 •

edited

Loading

lminiero Mar 2, 2022 •

edited

Loading

lminiero Mar 2, 2022 •

edited

Loading