[sr-dev] [kamailio/kamailio] rtp_media_server: adding module (#1701)

[Henning Westerholt]

henningw commented on this pull request.

I have added several small comments about places that I noticed, but as this is a large and complex module, more time is needed. Some general remarks:

- i noticed some differences in several module parts with regards to documentation and error handling. Some parts are quite extensive, and some parts lacks some checks. This are probably parts that are still changing - just an observation

- please check the return status consistently for functions (i could imagine that the external library functions like init(), destroy(), create_decoder(), attach() etc.. can fail)
- Replace OpenSER with Kamailio in the comments
- remove the ~150kb music file, maybe there is the possibility to add a link in the docs instead to a source
- all functions and variables that are not needed in a global scope (e.g. because they are accessed from other modules or cfg script) should be made static, to restrict their visibility to the module compilation unit. Variables can of course also moved to the respective functions, this is even better.

I have made more comments at several places directly in the code.

> +	ms_connection_helper_start(&h);
+	ms_connection_helper_link(&h, m1->ms_rtprecv, -1, 0);
+	ms_connection_helper_link(&h, m2->ms_rtpsend, 0, -1);
+
+	// direction 2
+	ms_connection_helper_start(&h);
+	ms_connection_helper_link(&h, m2->ms_rtprecv, -1, 0);
+	ms_connection_helper_link(&h, m1->ms_rtpsend, 0, -1);
+
+	ms_ticker_attach_multiple(
+			m1->ms_ticker, m1->ms_rtprecv, m2->ms_rtprecv, NULL);
+
+	return 1;
+}
+
+#define MS_UNUSED(x) ((void)(x))

What is the purpose of this define?

> +	}
+	ms_factory_init_plugins(f);
+	ms_factory_enable_statistics(f, TRUE);
+	ms_factory_reset_statistics(f);
+	return f;
+}
+
+int rms_media_init()
+{
+	OrtpMemoryFunctions ortp_memory_functions;
+	ortp_memory_functions.malloc_fun = ptr_shm_malloc;
+	ortp_memory_functions.realloc_fun = ptr_shm_realloc;
+	ortp_memory_functions.free_fun = ptr_shm_free;
+	ortp_set_memory_functions(&ortp_memory_functions);
+	ortp_init();
+	vars = ortp_malloc(sizeof(shared_global_vars_t));

Why you use the ortp_malloc() for allocation of Kamailio module structures?
The malloc can also fail.

> +#define rms_media_h
+
+#include "../../core/mem/shm.h"
+#include "mediastreamer2/mediastream.h"
+#include "mediastreamer2/msrtp.h"
+#include "mediastreamer2/mediastream.h"
+#include "mediastreamer2/dtmfgen.h"
+#include "mediastreamer2/msfileplayer.h"
+#include "mediastreamer2/msfilerec.h"
+#include "mediastreamer2/msrtp.h"
+#include "mediastreamer2/mstonedetector.h"
+#include "mediastreamer2/msfilter.h"
+//#include <mediastreamer2/mediastream.h>
+#include <ortp/ortp.h>
+#include <ortp/port.h>
+#define MS_UNUSED(x) ((void)(x))

see above

> +
+// https://tools.ietf.org/html/rfc4566
+// (protocol version)
+const char *sdp_v = "v=0\r\n";
+// (session name)
+const char *sdp_s = "s=-\r\n";
+// (time the session is active)
+const char *sdp_t = "t=0 0\r\n";
+//"a=rtpmap:101 telephone-event/8000\r\n"
+//"a=fmtp:101 0-15\r\n";
+//"a=rtpmap:0 PCMU/8000\r\n"
+//"a=rtpmap:8 PCMA/8000\r\n"
+//"a=rtpmap:96 opus/48000/2\r\n"
+//"a=fmtp:96 useinbandfec=1\r\n";
+
+static char *rms_shm_strdup(char *source)

Can't you use the core shm_str_dup() instead? 

> +	char sdp_o[128];
+	snprintf(
+			sdp_o, 128, "o=- 1028316687 1 IN IP4 %s\r\n", sdp_info->local_ip.s);
+	body->len += strlen(sdp_o);
+
+	// (connection information -- not required if included in all media)
+	char sdp_c[128];
+	snprintf(sdp_c, 128, "c=IN IP4 %s\r\n", sdp_info->local_ip.s);
+	body->len += strlen(sdp_c);
+
+	char sdp_m[128];
+	snprintf(sdp_m, 128, "m=audio %d RTP/AVP %d\r\n", sdp_info->udp_local_port,
+			payload_type_number);
+	body->len += strlen(sdp_m);
+
+	body->s = pkg_malloc(body->len + 1);

pkg_malloc can fail

> +{
+	if(rank == PROC_MAIN) {
+		int pid;
+		pid = fork_process(PROC_XWORKER, "RTP_media_server", 1);
+		if(pid < 0)
+			return -1;
+		if(pid == 0) {
+			rms_session_manage_loop();
+			return 0;
+		}
+	}
+	int rtn = 0;
+	return (rtn);
+}
+
+int rms_str_dup(str *dst, str *src, int shared)

please use the pkg_str_dup() and shm_str_dup() in the core

> +	return 1;
+error:
+	rms_sdp_info_free(sdp_info);
+	return 0;
+}
+
+static int rms_relay_call(struct sip_msg *msg)
+{
+	if(!tmb.t_relay(msg, NULL, NULL)) {
+		LM_INFO("t_ralay error\n");
+		return -1;
+	}
+	return 1;
+}
+
+str reply_headers = {0, 0};

unused

> +error:
+	rms_sdp_info_free(sdp_info);
+	return 0;
+}
+
+static int rms_relay_call(struct sip_msg *msg)
+{
+	if(!tmb.t_relay(msg, NULL, NULL)) {
+		LM_INFO("t_ralay error\n");
+		return -1;
+	}
+	return 1;
+}
+
+str reply_headers = {0, 0};
+str headers = str_init("Max-Forwards: 70" CRLF);

This and the following vars are only used in one functions. Please consider moving them to the respective functions, they don't need to be globally visible.

> +const char *sdp_s = "s=-\r\n";
+// (time the session is active)
+const char *sdp_t = "t=0 0\r\n";
+//"a=rtpmap:101 telephone-event/8000\r\n"
+//"a=fmtp:101 0-15\r\n";
+//"a=rtpmap:0 PCMU/8000\r\n"
+//"a=rtpmap:8 PCMA/8000\r\n"
+//"a=rtpmap:96 opus/48000/2\r\n"
+//"a=fmtp:96 useinbandfec=1\r\n";
+
+static char *rms_shm_strdup(char *source)
+{
+	char *copy;
+	if(!source)
+		return NULL;
+	copy = (char *)ortp_malloc(strlen(source) + 1);

see other comment

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/1701#pullrequestreview-170889197
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.kamailio.org/pipermail/sr-dev/attachments/20181101/3c74eab5/attachment-0001.html>