<p>Joshua Colp <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>(14 comments)</p><ul style="list-style: none; padding-left: 20px;"><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@933">Patch Set #11, Line 933:</a> <code style="font-family:monospace,monospace"> case AST_FRAME_MODEM:</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should the core be updated to treat this under the image stream and thus the same approach using for read/write callbacks used for T.38?</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/5876/11/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/5876/11/channels/pjsip/dialplan_functions.c@937">Patch Set #11, Line 937:</a> <code style="font-family:monospace,monospace"> if (!session->pending_media_state->topology) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">What if there is already a pending media state in progress? Should this go into a datastore on the channel which is initialized to the endpoint media topology?</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5876/11/channels/pjsip/dialplan_functions.c@1008">Patch Set #11, Line 1008:</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 (!data->session->pending_media_state->topology) {<br> data->session->pending_media_state->topology = ast_stream_topology_clone(data->session->endpoint->media.topology);<br> if (!data->session->pending_media_state->topology) {<br> return 0;<br> }<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Same here</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5876/11/channels/pjsip/dialplan_functions.c@1122">Patch Set #11, Line 1122:</a> <code style="font-family:monospace,monospace"> sip_session_response_cb, data->method, 1, NULL);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should this use the datastore I mentioned above for sending a session refresh?</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 style="white-space: pre-wrap; word-wrap: break-word;">Do we just want to go ahead and remove this?</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@944">Patch Set #11, Line 944:</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 port is 0, ignore this media stream */<br> if (!stream->desc.port) {<br> ast_debug(3, "Media stream '%s' is already declined\n",<br> ast_codec_media_type2str(session_media->type));<br> return 0;<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should this logic actually go into res_pjsip_session so the handler isn't even called?</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@1371">Patch Set #11, Line 1371:</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;"> stream = ast_stream_alloc(ast_codec_media_type2str(type), type);<br> if (!stream) {<br> return -1;<br> }<br><br> ast_stream_topology_set_stream(session->pending_media_state->topology, i, stream);<br><br> session_media = ast_sip_session_media_state_add(session, session->pending_media_state, ast_media_type_from_str(media), i);<br> if (!session_media) {<br> return -1;<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Document why these don't check for an existing stream or session media</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 style="white-space: pre-wrap; word-wrap: break-word;">This is a noop and should be removed, ast_sip_session_media_state_free does it.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_session.c@3408">Patch Set #11, Line 3408:</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 (is_stream_limitation_reached(ast_stream_get_type(stream), session->endpoint, type_streams)) {<br> ast_stream_set_state(stream, AST_STREAM_STATE_REMOVED);<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">On an answer it makes sense to decline as we have to. In that case should we even bother setting up session media and all that?</p><p style="white-space: pre-wrap; word-wrap: break-word;">On an offer we can just leave the streams out. In fact - in an offer does it make more sense to prune the pending topology ahead of time so it doesn't contain a number of streams exceeding the configured limits?</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@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 style="white-space: pre-wrap; word-wrap: break-word;">Should we do anything to the prior active media state we have cloned away to stop RTP or other things?</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 style="white-space: pre-wrap; word-wrap: break-word;">There's a t38 format in the core now, does it make sense to set a format capabilities here containing it so "core show channels" and other stuff can query and see?</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_t38.c@489">Patch Set #11, Line 489:</a> <code style="font-family:monospace,monospace">static struct ast_frame *t38_framehook_read(struct ast_channel *chan,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">If T.38 is moved to be a proper stream then the use of framehooks can just be limited to the control frames.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_t38.c@738">Patch Set #11, Line 738:</a> <code style="font-family:monospace,monospace"> pjmedia_sdp_media *stream = sdp->media[index];</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should this go back to being passed in?</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_t38.c@890">Patch Set #11, Line 890:</a> <code style="font-family:monospace,monospace"> pjmedia_sdp_media *remote_stream = remote->media[index];</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should this go back to being passed in?</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-Comment-Date: Wed, 21 Jun 2017 16:57:01 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>