<p>Richard Mudgett <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/5760">View Change</a></p><p>Patch set 5:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p>(8 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/5760/5/channels/chan_pjsip.c">File channels/chan_pjsip.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5760/5/channels/chan_pjsip.c@434">Patch Set #5, Line 434:</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;">*!<br> * \brief Determine if topologies have compatible formats<br> *<br> * This will return true if ANY formats in the topology are compatible with one another.<br> * It does not guarantee that each individual stream is compatible with the other streams.<br> *<br> * XXX When supporting true multistream, we will need to be sure to mark which streams from<br> * top1 are compatible with which streams from top2. Then the ones that are not compatible<br> * will need to be marked as "removed" so that they are negotiated as expected.<br> *<br> * \param top1 Topology 1<br> * \param top2 Topology 2<br> * \retval 1 The topologies have at least one compatible format<br> * \retval 0 The topologies have no compatible formats or an error occurred.<br> */<br>static int compatible_formats_exist(struct ast_stream_topology *top, struct ast_format_cap *cap)<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The doxygen no longer agrees with the prototype.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/5760/5/channels/pjsip/dialplan_functions.c">File channels/pjsip/dialplan_functions.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5760/5/channels/pjsip/dialplan_functions.c@952">Patch Set #5, Line 952:</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;">static int media_offer_read_av(struct ast_sip_session *session, char *buf,<br> size_t len, enum ast_media_type media_type)<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Hmm. Maybe this should only get the first stream of type and return that codec list. The current code will generate an accumulated codec list of all streams of the type. Any common codecs of the streams will get listed multiple times.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/5760/5/include/asterisk/res_pjsip.h">File include/asterisk/res_pjsip.h:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5760/5/include/asterisk/res_pjsip.h@683">Patch Set #5, Line 683:</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;"> /*! Capabilities in topology form */<br> struct ast_stream_topology *topology;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Since this is for master only, I'd add this next to the codecs member since they represent the same information and it avoids possible struct padding.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/5760/5/res/res_pjsip/pjsip_configuration.c">File res/res_pjsip/pjsip_configuration.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5760/5/res/res_pjsip/pjsip_configuration.c@2068">Patch Set #5, Line 2068:</a> <code style="font-family:monospace,monospace"> ao2_cleanup(endpoint->media.codecs);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Need to free the new topology member here too.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/5760/5/res/res_pjsip_sdp_rtp.c">File res/res_pjsip_sdp_rtp.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5760/5/res/res_pjsip_sdp_rtp.c@361">Patch Set #5, Line 361:</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 (direct_media_enabled) {<br> ast_format_cap_get_compatible(session->endpoint->media.codecs, session->direct_media_cap, caps);<br> } else {<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">What was the reason format_cap_only_type() wasn't restored too?</p><p style="white-space: pre-wrap; word-wrap: break-word;">Actually I see that it doesn't really matter that caps has codecs other than the media_type in it so the function is not needed anyway. The only place it has an effect is when we report in a warning message about no joint codecs.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5760/5/res/res_pjsip_sdp_rtp.c@405">Patch Set #5, Line 405:</a> <code style="font-family:monospace,monospace"> ast_format_cap_replace_from_cap(req_caps, joint, AST_MEDIA_TYPE_UNKNOWN);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This should be instead<br>ast_stream_set_formats(req_stream, joint);</p><p style="white-space: pre-wrap; word-wrap: break-word;">Meaning ast_stream_set_formats() can be pulled out of both if (!req_stream) clauses and placed after the if statement as common code and the req_caps variable is not used anymore.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/5760/5/res/res_pjsip_session.c">File res/res_pjsip_session.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5760/5/res/res_pjsip_session.c@1781">Patch Set #5, Line 1781:</a> <code style="font-family:monospace,monospace"> req_stream = ast_stream_topology_get_stream(req_topology, i);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Need check if req_stream is declined and skip it.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5760/5/res/res_pjsip_session.c@1793">Patch Set #5, Line 1793:</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;"> ast_format_cap_append_from_cap(req_cap,<br> endpoint->media.codecs, AST_MEDIA_TYPE_UNKNOWN);<br> /* replace instances of joint caps equivalents in req_caps */<br> ast_format_cap_replace_from_cap(req_cap, joint_cap,<br> AST_MEDIA_TYPE_UNKNOWN);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This code is modifying req_cap which is the caps from the passed in req_topology. Probably shouldn't do that. Also this code is throwing all media type codecs into the mix when it should be media type specific codecs only.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Operate on the clone_stream caps instead.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/5760">change 5760</a>. To unsubscribe, 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/5760"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I0358ea3108e2c06ae43308abddf216a23deccb90 </div>
<div style="display:none"> Gerrit-Change-Number: 5760 </div>
<div style="display:none"> Gerrit-PatchSet: 5 </div>
<div style="display:none"> Gerrit-Owner: Mark Michelson <mmichelson@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Mark Michelson <mmichelson@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 15 Jun 2017 20:02:27 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>