<p>Richard Mudgett <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/5876">View Change</a></p><p>Patch set 11:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p>(33 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/5876/11//COMMIT_MSG">Commit Message:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5876/11//COMMIT_MSG@36">Patch Set #11, Line 36:</a> <code style="font-family:monospace,monospace">within the resulting the SDP.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">within the resulting SDP.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/5876/11/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/5876/11/channels/chan_pjsip.c@523">Patch Set #11, Line 523:</a> <code style="font-family:monospace,monospace"> if (!(pvt = ao2_alloc(sizeof(*pvt), NULL))) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should we not keep the pvt dtor anyway in case the pvt ever gets any members again? As it is, there are no members in the pvt struct.</p><ul><li>Maybe we should add a dummy member so the struct isn't empty.</li><li>Was the ao2 lock ever needed for this struct?</li></ul></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5876/11/channels/chan_pjsip.c@565">Patch Set #11, Line 565:</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 (!topology) {<br> ao2_ref(caps, -1);<br> ast_channel_unlock(chan);<br> ast_hangup(chan);<br> return NULL;<br> }<br></pre></blockquote></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">Should be:<br>if (!topology || !caps) {<br> ao2_cleanup(caps);<br> ast_stream_topology_free(topology);<br> ast_channel_unlock(chan);<br> ast_hangup(chan);<br> return NULL;<br>}</pre></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5876/11/channels/chan_pjsip.c@572">Patch Set #11, Line 572:</a> <code style="font-family:monospace,monospace"> ast_channel_set_stream_topology(chan, topology);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This probably should be done after stage snapshot with the ast_channel_native_formats_set() call since it is similar.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5876/11/channels/chan_pjsip.c@574">Patch Set #11, Line 574:</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 (!caps) {<br> ast_channel_unlock(chan);<br> ast_hangup(chan);<br> return NULL;<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This can now be deleted with above change.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5876/11/channels/chan_pjsip.c@881">Patch Set #11, Line 881:</a> <code style="font-family:monospace,monospace"> if (stream_num >= 0) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should the else clause get the default stream since chan_pjsip_write() calls with -1?</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5876/11/channels/chan_pjsip.c@887">Patch Set #11, Line 887:</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 (!media) {<br> return 0;<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This check is redundant with the switch cases.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5876/11/channels/chan_pjsip.c@897">Patch Set #11, Line 897:</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_log(LOG_WARNING, "Channel %s stream %d is of type '%s', not audio!\n",<br> ast_channel_name(ast), stream_num, ast_codec_media_type2str(media->type));<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I suspect this warning and the same warning in video will spew spam when stream types change.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5876/11/channels/chan_pjsip.c@1418">Patch Set #11, Line 1418:</a> <code style="font-family:monospace,monospace"> AST_VECTOR_CALLBACK_VOID(&session->active_media_state->sessions, local_hold_set_state, held);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Looking at many places where the session_media pointer is obtained from the vector, it looks like the vector can have holes in it for possibly declined or unsupported streams. Therefor, local_hold_set_state() needs to be NULL tolerant.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5876/11/channels/chan_pjsip.c@1441">Patch Set #11, Line 1441:</a> <code style="font-family:monospace,monospace"> struct ast_stream_topology *topology;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This member doesn't appear to be used.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5876/11/channels/chan_pjsip.c@1486">Patch Set #11, Line 1486:</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 {<br> /* The topology change failed, so drop the current pending media state */<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">What about 1xx responses? Shouldn't they be ignored?</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5876/11/channels/chan_pjsip.c@1497">Patch Set #11, Line 1497:</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;"> struct topology_change_refresh_data *refresh_data = data;<br><br> if (ast_sip_session_refresh(refresh_data->session, NULL, NULL, on_topology_change_response,<br> AST_SIP_SESSION_REFRESH_METHOD_INVITE, 1, refresh_data->media_state)) {<br> refresh_data->media_state = NULL;<br> topology_change_refresh_data_free(refresh_data);<br> return -1;<br> }<br><br> refresh_data->media_state = NULL;<br> topology_change_refresh_data_free(refresh_data);<br> return 0;<br></pre></blockquote></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">This could be:<br>{<br> struct topology_change_refresh_data *refresh_data = data;<br> int ret;</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> ret = ast_sip_session_refresh(...);<br> refresh_data->media_state = NULL;<br> topology_change_refresh_data_fress(refresh_data);<br> return ret;<br>}</pre></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/5876/11/channels/pjsip/cli_commands.c">File channels/pjsip/cli_commands.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5876/11/channels/pjsip/cli_commands.c@339">Patch Set #11, Line 339:</a> <code style="font-family:monospace,monospace">static int find_audio(struct ast_sip_session_media *session_media)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should be NULL tolerant of session_media.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5876/11/channels/pjsip/cli_commands.c@369">Patch Set #11, Line 369:</a> <code style="font-family:monospace,monospace"> ast_channel_lock(channel);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">The channel lock doesn't necessarily protect you when accessing session members. This is a CLI thread. Accessing the session is done by the channel's thread, session's serializer thread, and here by a CLI thread.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The session->active_media_state->sessions vector likely has reentrancy problems that were prevented by the old media ao2 container lock.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/5876/11/channels/pjsip/include/chan_pjsip.h">File channels/pjsip/include/chan_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/5876/11/channels/pjsip/include/chan_pjsip.h@38">Patch Set #11, Line 38:</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 The PJSIP channel driver pvt, stored in the \ref ast_sip_channel_pvt<br> * data structure<br> */<br>struct chan_pjsip_pvt {<br>};<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Do we just want to go ahead and remove this?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">To avoid reentrancy issues with the channel thread and the session serializer competing for access to the media structs maybe we should have an array of first media types and a copy of the media vector in the session here. These arrays could then be updated when the session's vector is updated by the serializer and the channel lock being held. Access to these vectors would be protected by the channel lock and access to the session's vectors would be protected by only the serializer thread accessing them.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/5876/11/include/asterisk/res_pjsip_session.h">File include/asterisk/res_pjsip_session.h:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5876/11/include/asterisk/res_pjsip_session.h@354">Patch Set #11, Line 354:</a> <code style="font-family:monospace,monospace"> * \param session_media The media session</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Spaces were used to indent here.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5876/11/include/asterisk/res_pjsip_session.h@387">Patch Set #11, Line 387:</a> <code style="font-family:monospace,monospace"> * \param session_media The media session</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Spaces were used to indent here.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/5876/11/main/channel.c">File main/channel.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5876/11/main/channel.c@10955">Patch Set #11, Line 10955:</a> <code style="font-family:monospace,monospace"> if (ast_stream_topology_equal(ast_channel_get_stream_topology(chan), topology)) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">What if we just want to change stream names in the topology only? We don't seem to be able to do that unless we change something else on the topology.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/5876/11/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/5876/11/res/res_pjsip_sdp_rtp.c@a1384">Patch Set #11, Line 1384:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">We seem to have lost this declined stream check. Is it really no longer needed?</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_sdp_rtp.c@1161">Patch Set #11, Line 1161:</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 this is a removed (or declined) stream OR if no formats exist then construct a minimal stream in SDP */<br> if (ast_stream_get_state(stream) == AST_STREAM_STATE_REMOVED || !ast_stream_get_formats(stream) ||<br> !ast_format_cap_count(ast_stream_get_formats(stream))) {<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">It should be a bug if the stream has no format caps or it is empty and the stream state is not AST_STREAM_STATE_REMOVED.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_sdp_rtp.c@1164">Patch Set #11, Line 1164:</a> <code style="font-family:monospace,monospace"> media->desc.transport = STR_RTP_AVP;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">We should use the same transport as the remote if we are declining a remote offered stream.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_sdp_rtp.c@1166">Patch Set #11, Line 1166:</a> <code style="font-family:monospace,monospace"> media->desc.port_count = 1;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Spaces used to indent.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/5876/11/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/5876/11/res/res_pjsip_session.c@306">Patch Set #11, Line 306:</a> <code style="font-family:monospace,monospace"> return AST_VECTOR_APPEND(&session->pending_media_state->read_callbacks, callback_state);</code></p><ul><li>Probably should have a comment that the vector elements are structs and not pointers to a struct.</li></ul><ul><li>Probably should emphasize that where the vector is declared as well.</li></ul></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_session.c@399">Patch Set #11, Line 399:</a> <code style="font-family:monospace,monospace"> session_media = ao2_alloc(sizeof(*session_media), session_media_dtor);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Is the ao2 lock needed?</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_session.c@414">Patch Set #11, Line 414:</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_stream_get_state(ast_stream_topology_get_stream(media_state->topology, position)) != AST_STREAM_STATE_REMOVED &&<br> !media_state->default_session[type]) {<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">We can avoid some API calls by swapping the order of the && expressions: B && A instead of A && B</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_session.c@422">Patch Set #11, Line 422:</a> <code style="font-family:monospace,monospace">static int is_stream_limitation_reached(enum ast_media_type type, const struct ast_sip_endpoint *endpoint, int *type_streams)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">As far as stream limits are concerned, we will need to have protection from the system requesting more stream positions than PJPROJECT can support. This is a hard limit since PJPROJECT uses compile time arrays.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The SFU ConfBridge doesn't know about any such limit and if more than 15 participants are in the bridge it will try to request over 16 streams.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_session.c@430">Patch Set #11, Line 430:</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;"> case AST_MEDIA_TYPE_TEXT:<br> /* We don't have an option for image (T.38) and text streams<br> * so we cap these at one<br> */<br></pre></blockquote></p><ul><li>AFAIK TEXT streams are not supported by res_pjsip so it should always be hitting the limit like with UNKNOWN.</li></ul><ul><li>The comment should be updated accordingly.</li></ul></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_session.c@699">Patch Set #11, Line 699:</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_state = session->active_media_state;<br> session->active_media_state = session->pending_media_state;<br><br> /* Active and pending flip flop as needed, so clear the former active for the next pending change */<br> session->pending_media_state = media_state;<br> sip_session_media_state_reset(session->pending_media_state);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is broken up strangely. How about:</p><p style="white-space: pre-wrap; word-wrap: break-word;">/* Active and pending flip flop as needed... */<br>SWAP(session->active_media_state, session->pending_media_state);<br>sip_session_media_state_reset(session->pending_media_state);</p><p style="white-space: pre-wrap; word-wrap: break-word;">And delete the no longer used media_state local variable.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_session.c@1681">Patch Set #11, Line 1681:</a> <code style="font-family:monospace,monospace"> ast_stream_topology_free(session->pending_media_state->topology);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This is a noop and should be removed, ast_sip_session_media_state_free does</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">No, this is a use after free and ast_sip_session_media_state_free() currently doesn't free the topology.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_session.c@2132">Patch Set #11, Line 2132:</a> <code style="font-family:monospace,monospace"> ast_stream_set_formats(clone_stream, joint_cap);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">joint_cap is ref leaked after ast_stream_set_format().</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_t38.c">File res/res_pjsip_t38.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_t38.c@300">Patch Set #11, Line 300:</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 (status.code == 100) {<br> return 0;<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ha. This callback does get 1xx responses that should be ignored. I pinged this elsewhere and this is confirmation that we need to.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_t38.c@312">Patch Set #11, Line 312:</a> <code style="font-family:monospace,monospace"> session_media = session->active_media_state->default_session[AST_MEDIA_TYPE_IMAGE];</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Should we do anything to the prior active media state we have cloned away t</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Active streams that subsequently get declined (such as switching to T.38) should get the rtp instance destroyed. I'm not sure if the code explicitly does this though.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_t38.c@346">Patch Set #11, Line 346:</a> <code style="font-family:monospace,monospace"> ast_stream_set_state(stream, AST_STREAM_STATE_SENDRECV);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">There's a t38 format in the core now, does it make sense to set a format ca</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Yes. I think a NULL or empty stream caps shouldn't be allowed if the stream is not declined.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/5876">change 5876</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/5876"/><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: I8afd8dd2eb538806a39b887af0abd046266e14c7 </div>
<div style="display:none"> Gerrit-Change-Number: 5876 </div>
<div style="display:none"> Gerrit-PatchSet: 11 </div>
<div style="display:none"> Gerrit-Owner: Joshua Colp <jcolp@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: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 21 Jun 2017 20:36:02 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>