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.

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

rtp_media_server: adding module #1701

Merged
merged 2 commits into from Nov 13, 2018

Conversation

Projects
None yet
4 participants
@jchavanton
Member

jchavanton commented Nov 1, 2018

Pre-Submission Checklist

  • Commit message has the format required by CONTRIBUTING guide
  • Commits are split per component (core, individual modules, libs, utils, ...)
  • Each component has a single commit (if not, squash them into one commit)
  • 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

  • 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();
        }
}

@jchavanton jchavanton force-pushed the jchavanton:rtp_media_server branch from 536628f to f11bf4e Nov 1, 2018

@henningw

This comment has been minimized.

Contributor

henningw commented Nov 1, 2018

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

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.

return 1;
}
#define MS_UNUSED(x) ((void)(x))

This comment has been minimized.

@henningw

henningw Nov 1, 2018

Contributor

What is the purpose of this define?

This comment has been minimized.

@jchavanton

jchavanton Nov 3, 2018

Member

I modified the includes for mediastreamer2 and included the file that was defining this macro instead.

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));

This comment has been minimized.

@henningw

henningw Nov 1, 2018

Contributor

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

This comment has been minimized.

@jchavanton

jchavanton Nov 3, 2018

Member

no good reason, ortp_malloc is using shm_malloc, I replaced the 2 occurrences with shm_malloc for clarity from Kamailio module context

//#include <mediastreamer2/mediastream.h>
#include <ortp/ortp.h>
#include <ortp/port.h>
#define MS_UNUSED(x) ((void)(x))

This comment has been minimized.

@henningw

henningw Nov 1, 2018

Contributor

see above

This comment has been minimized.

@jchavanton

jchavanton Nov 3, 2018

Member

removed, since we now include mscommon.h

//"a=rtpmap:96 opus/48000/2\r\n"
//"a=fmtp:96 useinbandfec=1\r\n";
static char *rms_shm_strdup(char *source)

This comment has been minimized.

@henningw

henningw Nov 1, 2018

Contributor

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

This comment has been minimized.

@jchavanton

jchavanton Nov 3, 2018

Member

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.

This comment has been minimized.

@jchavanton

jchavanton Nov 3, 2018

Member

Maybe we could add another helper function in the core ?

This comment has been minimized.

@jchavanton

jchavanton Nov 3, 2018

Member

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 ...

payload_type_number);
body->len += strlen(sdp_m);
body->s = pkg_malloc(body->len + 1);

This comment has been minimized.

@henningw

henningw Nov 1, 2018

Contributor

pkg_malloc can fail

This comment has been minimized.

@jchavanton

jchavanton Nov 3, 2018

Member

I did fix this one and verify that none were left unchecked

return (rtn);
}
int rms_str_dup(str *dst, str *src, int shared)

This comment has been minimized.

@henningw

henningw Nov 1, 2018

Contributor

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

This comment has been minimized.

@jchavanton

jchavanton Nov 3, 2018

Member

Same topic as before, since they do not convert to \0 terminated char * they are not flexible in this case.

return 1;
}
str reply_headers = {0, 0};

This comment has been minimized.

@henningw

henningw Nov 1, 2018

Contributor

unused

}
str reply_headers = {0, 0};
str headers = str_init("Max-Forwards: 70" CRLF);

This comment has been minimized.

@henningw

henningw Nov 1, 2018

Contributor

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.

char *copy;
if(!source)
return NULL;
copy = (char *)ortp_malloc(strlen(source) + 1);

This comment has been minimized.

@henningw

henningw Nov 1, 2018

Contributor

see other comment

@jchavanton

This comment has been minimized.

Member

jchavanton commented Nov 1, 2018

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 there was further discussions about this at Cluecon2018 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 jchavanton force-pushed the jchavanton:rtp_media_server branch 2 times, most recently from 2b91534 to 1e07a18 Nov 4, 2018

@henningw

This comment has been minimized.

Contributor

henningw commented Nov 6, 2018

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.

@jchavanton

This comment has been minimized.

Member

jchavanton commented Nov 7, 2018

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

This comment has been minimized.

Member

jchavanton commented Nov 9, 2018

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

@SurendraPlivo

This comment has been minimized.

SurendraPlivo commented Nov 9, 2018

@henningw

This comment has been minimized.

Contributor

henningw commented Nov 9, 2018

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/

@jchavanton

This comment has been minimized.

Member

jchavanton commented Nov 9, 2018

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)>, 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 | Twitter: @plivo

Hi Surendra, RTP engine module is encoding the SDP and transferring it to an RTP engine server application.

RTP Media Server is starting an RTP endpoint 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 modules on the same call at the same time :) but you could use both modules on the same Kamailio instance/config

@SurendraPlivo

This comment has been minimized.

SurendraPlivo commented Nov 9, 2018

@henningw

This comment has been minimized.

Contributor

henningw commented Nov 12, 2018

@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.

@henningw henningw self-assigned this Nov 12, 2018

@miconda

This comment has been minimized.

Member

miconda commented Nov 12, 2018

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.

@jchavanton jchavanton force-pushed the jchavanton:rtp_media_server branch from 78d1090 to c74a9b4 Nov 13, 2018

@jchavanton jchavanton force-pushed the jchavanton:rtp_media_server branch from c74a9b4 to 2a76fdd Nov 13, 2018

@jchavanton

This comment has been minimized.

Member

jchavanton commented Nov 13, 2018

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.

@jchavanton jchavanton merged commit 32c363e into kamailio:master Nov 13, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@henningw

This comment has been minimized.

Contributor

henningw commented Nov 13, 2018

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment