<!-- Kamailio Pull Request Template -->
#### Pre-Submission Checklist - [x] Commit message has the format required by CONTRIBUTING guide - [x] Commits are split per component (core, individual modules, libs, utils, ...) - [x] Each component has a single commit (if not, squash them into one commit) - [x] No commits to README files for modules (changes must be done to docbook files in `doc/` subfolder, the README file is autogenerated)
#### Type Of Change - [x] New module
#### Checklist:
Tested with various sofphones : Ekiga, Linphone, CSipSimple
#### Description By combining Kamailio, oRTP and mediastreamer2 this module is providing some very good foundation to support RTP endpoint and various media processing features.
This seems like a great idea for various use cases like IoT, IVR or other specific needs.
Kamailio is handling everything related to SIP/SDP (the module is adding some SDP parsing) as well as providing a scripting engine.
oRTP is providing RTP endpoints compliant with (RFC 3550)
MediaStreamer2, even if written in C is quite a high level library, because it is implementing a framework for audio processing using graphs of filters, filters can be to do various things. Similar to playing with lego blocks :)
* Support for most free and some non free codecs can be added easily. * Work to bridge calls is already in progress in the module. * Mobile phone support ARM CPU * other embedded scenario should be supported * much more ...
Mediastream2 is creating one thread per call "msticker", this can work smoothly in Kamailio even if it is forking processes. Shared memory allocation is supported using wrapper around malloc/free used by the libraries.
Some extra work that needs to be done shorty * Syncronization with locks is not completed (minor task, only access to a linked list needs to be syncronized properly) * Memory leak not tested properly (not take the time to test with valgrind yet)
This project was started last year, I think it is time to submit it, I will surely find the time to do the extra work needed shortly.
Example config using the features already implemented ``` event_route[rms:start] { xnotice("[rms:start] play ...\n"); rms_play("/tmp/reference_8000.wav", "rms:after_play"); };
event_route[rms:after_play] { xnotice("[rms:after_play] play done...\n"); rms_hangup(); };
route { if (t_precheck_trans()) { t_check_trans(); exit; } t_check_trans(); if (is_method("INVITE") && !has_totag()) { if (!rms_answer()) { t_reply("503", "server error"); } }
if (is_method("BYE")){ rms_media_stop(); } } ``` You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/1701
-- Commit Summary --
* rtp_media_server: adding module
-- File Changes --
M src/Makefile.groups (6) A src/modules/rtp_media_server/Makefile (13) A src/modules/rtp_media_server/config_example/kamailio.cfg (56) A src/modules/rtp_media_server/doc/Makefile (4) A src/modules/rtp_media_server/doc/rtp_media_server.xml (42) A src/modules/rtp_media_server/doc/rtp_media_server_admin.xml (193) A src/modules/rtp_media_server/install_bc.sh (45) A src/modules/rtp_media_server/rms_media.c (320) A src/modules/rtp_media_server/rms_media.h (103) A src/modules/rtp_media_server/rms_sdp.c (301) A src/modules/rtp_media_server/rms_sdp.h (45) A src/modules/rtp_media_server/rtp_media_server.c (718) A src/modules/rtp_media_server/rtp_media_server.h (91) A src/modules/rtp_media_server/voice_file/Bach_10s_8000.wav (0)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/1701.patch https://github.com/kamailio/kamailio/pull/1701.diff
Thank you Julien for the contribution of this module. I will do a quick first review of the code.
The git master is currently in a feature freeze in preparation for the upcoming 5.2 release, so no new features can be integrated. So I would suggest to finish the mentioned remaining steps in your branch.
Did I understand the purpose of the module correctly, as its implements a RTP endpoint directly in Kamailio? This is kind of a new approach (for Kamailio) I think, the existing RTP handling modules only place the control channel into kamailio, and handle the RTP processing in a separate daemon.
It would suggest to write an e-mail to sr-dev, describing your module, the approach and getting feedback.
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
Hi Henning, thank you for taking some time to do the a review, I will address all the comments shortly.
Yes integrating a RTP endpoint directly in Kamailio may seems strange at first.
I did discuss this approach 2 years ago at Kamailio World and we discussed about this year at Cluecon with Daniel and Fred, the last feedback I got was that this was interesting.
By doing integration with a library we can benefit from the scripting engine and probably KEMI. This does not means that everyone should run this module on a Kamailio sever acting as a load balancer, of course CPU intensive operations like resampling and encoding may be taking place on the server.
I think the comments in this MR are hiting ser-dev mainling list but it make sense to write an email as well.
Much appreciated !
jchavanton commented on this pull request.
- 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))
I modified the includes for mediastreamer2 and included the file that was defining this macro instead.
jchavanton commented on this pull request.
- }
+ 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));
no good reason, ortp_malloc is using shm_malloc, I replaced the 2 occurrences with shm_malloc for clarity from Kamailio module context
jchavanton commented on this pull request.
+#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))
removed, since we now include mscommon.h
jchavanton commented on this pull request.
+// 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)
The reason is that shm_str_dup does not garanty terminated 'char*', this is one less thing to worry about. The module is not trying to optimize string like Kamailio is when parsing messages.
jchavanton commented on this pull request.
+// 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)
Maybe we could add another helper function in the core ?
jchavanton commented on this pull request.
+// 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)
I just did refactoring for now , we can clarify if it is best to use helper function from the core, add new ones or not ...
@jchavanton pushed 1 commit.
592b6d5 rtp_media_server: review part1 (will be squashed)
jchavanton commented on this pull request.
+{
+ 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)
Same topic as before, since they do not convert to `\0` terminated `char *` they are not flexible in this case.
jchavanton commented on this pull request.
- 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);
check that none was left unchecked
Hello Julien, thank you for updating the pull request. I also saw your message on sr-dev, so far no reply from the other developers. About the code - I will have another closer look on Friday. if there are no objections by then from the list we can discuss about integrating it into the master branch.
Thanks, this is good news !
So far there was only a brief discussion in slack, but nothing valuable enough to share in the mailing list. Mainly I shared my optimism about the fact that these libs are also designed to build server side components. The very minimalist feature set currently included was questioned, I believe that extending the features is not such a big endeavor considering the architecture of the routing script and the libraries.
I still need to address some comments you have made.
@jchavanton pushed 1 commit.
78d1090 rtp_media_server: review part2 (will be squashed)
revisited the synchronization and added a few more error handling ... We are sure we want to remove the small voice file ? `157K Bach_10s_8000.wav`
Just curious to know that rtp_media_server module will not conflict with rtpengine module.
On Fri, 9 Nov 2018 at 12:11 PM, Julien Chavanton notifications@github.com wrote:
revisited the synchronization and added a few more error handling ... We are sure we want to remove the small voice file ? 157K Bach_10s_8000.wav
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/kamailio/kamailio/pull/1701#issuecomment-437265721, or mute the thread https://github.com/notifications/unsubscribe-auth/AZoKAh3lBVGahBWPHLQkyWSn-BvSAHR1ks5utSOPgaJpZM4YF7gV .
We are sure we want to remove the small voice file ? 157K Bach_10s_8000.wav
I would just adding a link in the README to e.g. this, they provide voice files in different languages: http://www.voiptroubleshooter.com/open_speech/
Just curious to know that rtp_media_server module will not conflict with rtpengine module. On Fri, 9 Nov 2018 at 12:11 PM, Julien Chavanton ***@***.***> wrote: revisited the synchronization and added a few more error handling ... We are sure we want to remove the small voice file ? 157K Bach_10s_8000.wav — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <[#1701 (comment)](https://github.com/kamailio/kamailio/pull/1701#issuecomment-437265721)%3E, or mute the thread https://github.com/notifications/unsubscribe-auth/AZoKAh3lBVGahBWPHLQkyWSn-BvSAHR1ks5utSOPgaJpZM4YF7gV . -- Surendra Tiwari VoIP Application Developer, Plivo Inc., 201 Mission Street, *Suite 230*, San Francisco - 94105, USA Web: [www.plivo.com](http://www.plivo.com) | Twitter: @plivo
Hi Surendra, RTP engine module is encoding the SDP and transferring it to RTP engine server application.
RTP Media Server is starting an RTP enpoint in a thread from a Kamailio forked process.
There nothing related that can conflict, expect the fact that they will both play with the SDP, of course you can not use both module on the same call at the same time :) but you could use both module on the same Kamailio instance/config
ohk. Because what I will try with module by converting out of band dtmf to in band dtmf.
On Fri, 9 Nov 2018 at 12:28 PM, Julien Chavanton notifications@github.com wrote:
Just curious to know that rtp_media_server module will not conflict with rtpengine module. On Fri, 9 Nov 2018 at 12:11 PM, Julien Chavanton ***@***.***> wrote: revisited the synchronization and added a few more error handling ... We are sure we want to remove the small voice file ? 157K Bach_10s_8000.wav — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1701 (comment) https://github.com/kamailio/kamailio/pull/1701#issuecomment-437265721>, or mute the thread https://github.com/notifications/unsubscribe-auth/AZoKAh3lBVGahBWPHLQkyWSn-B... . -- Surendra Tiwari VoIP Application Developer, Plivo Inc., 201 Mission Street, https://maps.google.com/?q=201+Mission+Street,++Suite+230+,+San+Francisco+-+94105,+USA&entry=gmail&source=g*Suite 230 https://maps.google.com/?q=201+Mission+Street,++Suite+230+,+San+Francisco+-+94105,+USA&entry=gmail&source=g*, San Francisco - 94105, USA https://maps.google.com/?q=201+Mission+Street,++Suite+230+,+San+Francisco+-+94105,+USA&entry=gmail&source=g Web: www.plivo.com | Twitter: @plivo https://github.com/plivo
Hi Surendra, RTP engine module is encoding the SDP and transferring it to RTP engine server application.
RTP Media Server is starting an RTP enpoint in a thread from a Kamailio forked process.
There nothing related that can conflict, expect the fact that they will both play with the SDP, of course you can not use both module on the same call at the same time :) but you could use both module on the same Kamailio instance/config
— You are receiving this because you commented.
Reply to this email directly, view it on GitHub https://github.com/kamailio/kamailio/pull/1701#issuecomment-437268557, or mute the thread https://github.com/notifications/unsubscribe-auth/AZoKAr5Qsrri6VoDmoEGUGTtHT0rQIqJks5utSeGgaJpZM4YF7gV .
ohk. Because what I will try with module by converting out of band dtmf to in band dtmf. On Fri, 9 Nov 2018 at 12:28 PM, Julien Chavanton notifications@github.com wrote: Just curious to know that rtp_media_server module will not conflict with rtpengine module. On Fri, 9 Nov 2018 at 12:11 PM, Julien Chavanton ***@***.***> wrote: revisited the synchronization and added a few more error handling ... We are sure we want to remove the small voice file ? 157K Bach_10s_8000.wav — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1701 (comment) <[#1701 (comment)](https://github.com/kamailio/kamailio/pull/1701#issuecomment-437265721)%3E%3E, or mute the thread https://github.com/notifications/unsubscribe-auth/AZoKAh3lBVGahBWPHLQkyWSn-B... . -- Surendra Tiwari VoIP Application Developer, Plivo Inc., 201 Mission Street, https://maps.google.com/?q=201+Mission+Street,++Suite+230+,+San+Francisco+-+94105,+USA&entry=gmail&source=g*Suite 230 https://maps.google.com/?q=201+Mission+Street,++Suite+230+,+San+Francisco+-+94105,+USA&entry=gmail&source=g*, San Francisco - 94105, USA https://maps.google.com/?q=201+Mission+Street,++Suite+230+,+San+Francisco+-+94105,+USA&entry=gmail&source=g Web: [www.plivo.com](http://www.plivo.com) | Twitter: @plivo https://github.com/plivo Hi Surendra, RTP engine module is encoding the SDP and transferring it to RTP engine server application. RTP Media Server is starting an RTP enpoint in a thread from a Kamailio forked process. There nothing related that can conflict, expect the fact that they will both play with the SDP, of course you can not use both module on the same call at the same time :) but you could use both module on the same Kamailio instance/config — You are receiving this because you commented. Reply to this email directly, view it on GitHub <[#1701 (comment)](https://github.com/kamailio/kamailio/pull/1701#issuecomment-437268557)%3E, or mute the thread https://github.com/notifications/unsubscribe-auth/AZoKAr5Qsrri6VoDmoEGUGTtHT0rQIqJks5utSeGgaJpZM4YF7gV . -- Surendra Tiwari VoIP Application Developer, Plivo Inc., 201 Mission Street, *Suite 230*, San Francisco - 94105, USA Web: [www.plivo.com](http://www.plivo.com) | Twitter: @plivo
Not sure I understand your last comment
@jchavanton can you remove the WAV file as well? As there were no comments from other developers I can then add the module to the master branch. I will grant you access to do the further changes directly in the repository.
Although I didn't have time to look at the code, I am fine with merging it, being a lot of time till 5.3 release that allows testing and tuning, if someone is interested.
Actually, I wanted to write here to just say that @jchavanton has write access to kamailio already. So he can also merge and commit already.
I believe everything in the review was addressed
I did remove the audio file and added a link to download one.
I added another commit to support multiple actions this will be needed by some action like DTMF capture that could interrupt play and probably many other future commands.
Merged #1701 into master.
Thank you!
-
Daniel-Constantin Mierla -
Henning Westerholt
-
Julien Chavanton
-
Surendra Tiwari