<p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The license agreement question also needs to be answered.</p><p>Patch set 1:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4; color: #000000;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/13451">View Change</a></p><p>21 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/13451/1//COMMIT_MSG">Commit Message:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13451/1//COMMIT_MSG@9">Patch Set #1, Line 9:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">Changes are:<br>1. chan_pjsip - added write_text function to the pjsip_tech driver<br>Added case for AST_FRAME_TEXT in chan_pjsip_write_stream<br>Added case to handle text in chan_pjsip_write<br><br>2. pjsip/dialplan_functions.c: added logic to handle media for TEXT<br><br>3. res_pjsip.h: added variables to deal with endpoint media<br><br>4. channel.c: added logic to add a stream_num for a frame<br><br>5. frame.c: added code to handle and fix issues with red.<br><br>6. res_pjsip_sdp_rtp.c: added logic to setup SDP.<br>Added logic to handle tos and cos text<br>Added logic to RED for red enabled<br><br>7. res_pjsip_session.c: aded logic for max_text_streams<br><br>8. res_rtp_asterisk.c: Added logic to hanle Red and dealing with<br>a repeating RTP issue<br><br>9. pjsip_configuration.c: Added logic to handle text for PJSIP library<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The commit message doesn't need a history of all these changes</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13451/1//COMMIT_MSG@38">Patch Set #1, Line 38:</a> <code style="font-family:monospace,monospace">Seth Marks and Corey Wysong</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Based on this this code is from multiple people. Do those individuals have license agreements on file?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/13451/1/channels/chan_pjsip.c">File channels/chan_pjsip.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13451/1/channels/chan_pjsip.c@997">Patch Set #1, Line 997:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> } else if(session->endpoint->media.red_enabled) {<br> ast_rtp_red_buffer(media->rtp, frame);<br> } else if (media->write_callback) {<br> res = media->write_callback(session, media, frame);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Indentation</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13451/1/channels/chan_pjsip.c@1025">Patch Set #1, Line 1025:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> if (frame->frametype == AST_FRAME_TEXT && frame->stream_num != -1){<br> return chan_pjsip_write_stream(ast, frame->stream_num, frame);<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Is there a reason this doesn't just use chan_pjsip_write_stream for the write_text callback?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/13451/1/channels/pjsip/dialplan_functions.c">File channels/pjsip/dialplan_functions.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13451/1/channels/pjsip/dialplan_functions.c@579">Patch Set #1, Line 579:</a> <code style="font-family:monospace,monospace"> } else if (ast_strlen_zero(field) || !strcmp(field, "text")){</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I don't think this should use text if a field has not been specified, it should behave as it did before</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13451/1/channels/pjsip/dialplan_functions.c@580">Patch Set #1, Line 580:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> media = session->active_media_state->default_session[AST_MEDIA_TYPE_TEXT];<br> } else {<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Indentation</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/13451/1/include/asterisk/res_pjsip.h">File include/asterisk/res_pjsip.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13451/1/include/asterisk/res_pjsip.h@712">Patch Set #1, Line 712:</a> <code style="font-family:monospace,monospace"> /*! DSCP indicate if text stream supports RED */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Indentation, and this is just a bit to indicate that RED is enabled</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13451/1/include/asterisk/res_pjsip.h@713">Patch Set #1, Line 713:</a> <code style="font-family:monospace,monospace"> unsigned int red_enabled; </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Redness</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/13451/1/main/channel.c">File main/channel.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13451/1/main/channel.c@4701">Patch Set #1, Line 4701:</a> <code style="font-family:monospace,monospace"> f.stream_num = ast_stream_get_position(ast_channel_get_default_stream(chan, AST_MEDIA_TYPE_TEXT));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think this should tolerate a default stream not being present</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/13451/1/main/frame.c">File main/frame.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13451/1/main/frame.c@363">Patch Set #1, Line 363:</a> <code style="font-family:monospace,monospace"> if(f->frametype == AST_FRAME_TEXT) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Space between if and (</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13451/1/main/frame.c@364">Patch Set #1, Line 364:</a> <code style="font-family:monospace,monospace"> ((char*)out->data.ptr)[out->datalen] = '\0';</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why has this been changed to be null terminated?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/13451/1/res/res_pjsip/pjsip_configuration.c">File res/res_pjsip/pjsip_configuration.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13451/1/res/res_pjsip/pjsip_configuration.c@1919">Patch Set #1, Line 1919:</a> <code style="font-family:monospace,monospace"> ast_sorcery_object_field_register_custom(sip_sorcery, "endpoint", "tos_text", "0", tos_handler, tos_text_to_str, NULL, 0, 0);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">[1] https://alembic.sqlalchemy.org/en/latest/tutorial.html#create-a-migration-script</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13451/1/res/res_pjsip/pjsip_configuration.c@1922">Patch Set #1, Line 1922:</a> <code style="font-family:monospace,monospace"> ast_sorcery_object_field_register(sip_sorcery, "endpoint", "cos_text", "5", OPT_UINT_T, 0, FLDSET(struct ast_sip_endpoint, media.cos_text)); </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Redness</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13451/1/res/res_pjsip/pjsip_configuration.c@1962">Patch Set #1, Line 1962:</a> <code style="font-family:monospace,monospace"> 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)); </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Redness</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/13451/1/res/res_pjsip_sdp_rtp.c">File res/res_pjsip_sdp_rtp.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13451/1/res/res_pjsip_sdp_rtp.c@349">Patch Set #1, Line 349:</a> <code style="font-family:monospace,monospace"> if(strncasecmp(name, "RED", 3) == 0)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This needs to follow the coding guidelines[1].</p><p style="white-space: pre-wrap; word-wrap: break-word;">[1] https://wiki.asterisk.org/wiki/display/AST/Coding+Guidelines</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13451/1/res/res_pjsip_sdp_rtp.c@365">Patch Set #1, Line 365:</a> <code style="font-family:monospace,monospace"> session->endpoint->media.red_enabled=1;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13451/1/res/res_pjsip_sdp_rtp.c@2189">Patch Set #1, Line 2189:</a> <code style="font-family:monospace,monospace"> ast_sip_session_unregister_sdp_handler(&text_sdp_handler, STR_TEXT);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Indentation</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13451/1/res/res_pjsip_sdp_rtp.c@2236">Patch Set #1, Line 2236:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> if (ast_sip_session_register_sdp_handler(&text_sdp_handler, STR_TEXT)) {<br> ast_log(LOG_ERROR, "Unable to register SDP handler for %s stream type\n", STR_TEXT);<br> goto end;<br> }<br> <br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Redness and indentation</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/13451/1/res/res_rtp_asterisk.c">File res/res_rtp_asterisk.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13451/1/res/res_rtp_asterisk.c@7131">Patch Set #1, Line 7131:</a> <code style="font-family:monospace,monospace"> if(rtp->f.datalen <= 0) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Can you explain why these were added?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13451/1/res/res_rtp_asterisk.c@8063">Patch Set #1, Line 8063:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> if(!rtp || !rtp->red) {<br> return (0);<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why was this added?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13451/1/res/res_rtp_asterisk.c@8068">Patch Set #1, Line 8068:</a> <code style="font-family:monospace,monospace"> if (rtp && rtp->red && rtp->red->t140.datalen > 0) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Same here</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/13451">change 13451</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/c/asterisk/+/13451"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 16 </div>
<div style="display:none"> Gerrit-Change-Id: I6b2ad94a065680ae9c4c933d338989e4b8c3a505 </div>
<div style="display:none"> Gerrit-Change-Number: 13451 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Bharat Ramaswamy-Nandakumar <bharat@indigital.net> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Sun, 05 Jan 2020 22:44:31 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>