[Asterisk-code-review] Added RTT Support for PJSIP (asterisk[16])
Joshua Colp
asteriskteam at digium.com
Sun Jan 5 16:44:31 CST 2020
Joshua Colp has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/13451 )
Change subject: Added RTT Support for PJSIP
......................................................................
Patch Set 1: Code-Review-1
(21 comments)
As this is a new feature for inclusion into release versions, such as 16 and 17, test coverage has to exist. From a basic level that would be SDP negotiation but actual testing would also be good. Without test coverage this can only be included into the master branch, which would become Asterisk 18 in the future.
The license agreement question also needs to be answered.
https://gerrit.asterisk.org/c/asterisk/+/13451/1//COMMIT_MSG
Commit Message:
https://gerrit.asterisk.org/c/asterisk/+/13451/1//COMMIT_MSG@9
PS1, Line 9: Changes are:
: 1. chan_pjsip - added write_text function to the pjsip_tech driver
: Added case for AST_FRAME_TEXT in chan_pjsip_write_stream
: Added case to handle text in chan_pjsip_write
:
: 2. pjsip/dialplan_functions.c: added logic to handle media for TEXT
:
: 3. res_pjsip.h: added variables to deal with endpoint media
:
: 4. channel.c: added logic to add a stream_num for a frame
:
: 5. frame.c: added code to handle and fix issues with red.
:
: 6. res_pjsip_sdp_rtp.c: added logic to setup SDP.
: Added logic to handle tos and cos text
: Added logic to RED for red enabled
:
: 7. res_pjsip_session.c: aded logic for max_text_streams
:
: 8. res_rtp_asterisk.c: Added logic to hanle Red and dealing with
: a repeating RTP issue
:
: 9. pjsip_configuration.c: Added logic to handle text for PJSIP library
The commit message doesn't need a history of all these changes
https://gerrit.asterisk.org/c/asterisk/+/13451/1//COMMIT_MSG@38
PS1, Line 38: Seth Marks and Corey Wysong
Based on this this code is from multiple people. Do those individuals have license agreements on file?
https://gerrit.asterisk.org/c/asterisk/+/13451/1/channels/chan_pjsip.c
File channels/chan_pjsip.c:
https://gerrit.asterisk.org/c/asterisk/+/13451/1/channels/chan_pjsip.c@997
PS1, Line 997: } else if(session->endpoint->media.red_enabled) {
: ast_rtp_red_buffer(media->rtp, frame);
: } else if (media->write_callback) {
: res = media->write_callback(session, media, frame);
Indentation
https://gerrit.asterisk.org/c/asterisk/+/13451/1/channels/chan_pjsip.c@1025
PS1, Line 1025: if (frame->frametype == AST_FRAME_TEXT && frame->stream_num != -1){
: return chan_pjsip_write_stream(ast, frame->stream_num, frame);
: }
Is there a reason this doesn't just use chan_pjsip_write_stream for the write_text callback?
https://gerrit.asterisk.org/c/asterisk/+/13451/1/channels/pjsip/dialplan_functions.c
File channels/pjsip/dialplan_functions.c:
https://gerrit.asterisk.org/c/asterisk/+/13451/1/channels/pjsip/dialplan_functions.c@579
PS1, Line 579: } else if (ast_strlen_zero(field) || !strcmp(field, "text")){
I don't think this should use text if a field has not been specified, it should behave as it did before
https://gerrit.asterisk.org/c/asterisk/+/13451/1/channels/pjsip/dialplan_functions.c@580
PS1, Line 580: media = session->active_media_state->default_session[AST_MEDIA_TYPE_TEXT];
: } else {
Indentation
https://gerrit.asterisk.org/c/asterisk/+/13451/1/include/asterisk/res_pjsip.h
File include/asterisk/res_pjsip.h:
https://gerrit.asterisk.org/c/asterisk/+/13451/1/include/asterisk/res_pjsip.h@712
PS1, Line 712: /*! DSCP indicate if text stream supports RED */
Indentation, and this is just a bit to indicate that RED is enabled
https://gerrit.asterisk.org/c/asterisk/+/13451/1/include/asterisk/res_pjsip.h@713
PS1, Line 713: unsigned int red_enabled;
Redness
https://gerrit.asterisk.org/c/asterisk/+/13451/1/main/channel.c
File main/channel.c:
https://gerrit.asterisk.org/c/asterisk/+/13451/1/main/channel.c@4701
PS1, Line 4701: f.stream_num = ast_stream_get_position(ast_channel_get_default_stream(chan, AST_MEDIA_TYPE_TEXT));
I think this should tolerate a default stream not being present
https://gerrit.asterisk.org/c/asterisk/+/13451/1/main/frame.c
File main/frame.c:
https://gerrit.asterisk.org/c/asterisk/+/13451/1/main/frame.c@363
PS1, Line 363: if(f->frametype == AST_FRAME_TEXT) {
Space between if and (
https://gerrit.asterisk.org/c/asterisk/+/13451/1/main/frame.c@364
PS1, Line 364: ((char*)out->data.ptr)[out->datalen] = '\0';
Why has this been changed to be null terminated?
https://gerrit.asterisk.org/c/asterisk/+/13451/1/res/res_pjsip/pjsip_configuration.c
File res/res_pjsip/pjsip_configuration.c:
https://gerrit.asterisk.org/c/asterisk/+/13451/1/res/res_pjsip/pjsip_configuration.c@1919
PS1, Line 1919: ast_sorcery_object_field_register_custom(sip_sorcery, "endpoint", "tos_text", "0", tos_handler, tos_text_to_str, NULL, 0, 0);
Each new added option should be put into pjsip.conf.sample and an Alembic file needs to be created for realtime[1] in contrib/ast-db-manage using the config.ini.sample file for configuration.
You also need to document them in res/res_pjsip.c in the XML. As it is the module won't allow these to exist since they haven't been documented.
[1] https://alembic.sqlalchemy.org/en/latest/tutorial.html#create-a-migration-script
https://gerrit.asterisk.org/c/asterisk/+/13451/1/res/res_pjsip/pjsip_configuration.c@1922
PS1, Line 1922: ast_sorcery_object_field_register(sip_sorcery, "endpoint", "cos_text", "5", OPT_UINT_T, 0, FLDSET(struct ast_sip_endpoint, media.cos_text));
Redness
https://gerrit.asterisk.org/c/asterisk/+/13451/1/res/res_pjsip/pjsip_configuration.c@1962
PS1, Line 1962: ast_sorcery_object_field_register(sip_sorcery, "endpoint", "max_text_streams", "1", OPT_UINT_T, 0, FLDSET(struct ast_sip_endpoint, media.max_text_streams));
Redness
https://gerrit.asterisk.org/c/asterisk/+/13451/1/res/res_pjsip_sdp_rtp.c
File res/res_pjsip_sdp_rtp.c:
https://gerrit.asterisk.org/c/asterisk/+/13451/1/res/res_pjsip_sdp_rtp.c@349
PS1, Line 349: if(strncasecmp(name, "RED", 3) == 0)
This needs to follow the coding guidelines[1].
[1] https://wiki.asterisk.org/wiki/display/AST/Coding+Guidelines
https://gerrit.asterisk.org/c/asterisk/+/13451/1/res/res_pjsip_sdp_rtp.c@365
PS1, Line 365: session->endpoint->media.red_enabled=1;
You can not modify or alter the endpoint. It is immutable and to be treated as read-only. If you need to maintain this information it has to be on the session itself.
https://gerrit.asterisk.org/c/asterisk/+/13451/1/res/res_pjsip_sdp_rtp.c@2189
PS1, Line 2189: ast_sip_session_unregister_sdp_handler(&text_sdp_handler, STR_TEXT);
Indentation
https://gerrit.asterisk.org/c/asterisk/+/13451/1/res/res_pjsip_sdp_rtp.c@2236
PS1, Line 2236: if (ast_sip_session_register_sdp_handler(&text_sdp_handler, STR_TEXT)) {
: ast_log(LOG_ERROR, "Unable to register SDP handler for %s stream type\n", STR_TEXT);
: goto end;
: }
:
Redness and indentation
https://gerrit.asterisk.org/c/asterisk/+/13451/1/res/res_rtp_asterisk.c
File res/res_rtp_asterisk.c:
https://gerrit.asterisk.org/c/asterisk/+/13451/1/res/res_rtp_asterisk.c@7131
PS1, Line 7131: if(rtp->f.datalen <= 0) {
Can you explain why these were added?
https://gerrit.asterisk.org/c/asterisk/+/13451/1/res/res_rtp_asterisk.c@8063
PS1, Line 8063: if(!rtp || !rtp->red) {
: return (0);
: }
Why was this added?
https://gerrit.asterisk.org/c/asterisk/+/13451/1/res/res_rtp_asterisk.c@8068
PS1, Line 8068: if (rtp && rtp->red && rtp->red->t140.datalen > 0) {
Same here
--
To view, visit https://gerrit.asterisk.org/c/asterisk/+/13451
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: I6b2ad94a065680ae9c4c933d338989e4b8c3a505
Gerrit-Change-Number: 13451
Gerrit-PatchSet: 1
Gerrit-Owner: Bharat Ramaswamy-Nandakumar <bharat at indigital.net>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Comment-Date: Sun, 05 Jan 2020 22:44:31 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20200105/9250458a/attachment.html>
More information about the asterisk-code-review
mailing list