[Asterisk-code-review] chan pjsip: Add support for multiple streams of the same type. (asterisk[master])
Joshua Colp
asteriskteam at digium.com
Wed Jun 21 11:57:01 CDT 2017
Joshua Colp has posted comments on this change. ( https://gerrit.asterisk.org/5876 )
Change subject: chan_pjsip: Add support for multiple streams of the same type.
......................................................................
Patch Set 11: Code-Review-1
(14 comments)
https://gerrit.asterisk.org/#/c/5876/11/channels/chan_pjsip.c
File channels/chan_pjsip.c:
https://gerrit.asterisk.org/#/c/5876/11/channels/chan_pjsip.c@933
PS11, Line 933: case AST_FRAME_MODEM:
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?
https://gerrit.asterisk.org/#/c/5876/11/channels/pjsip/dialplan_functions.c
File channels/pjsip/dialplan_functions.c:
https://gerrit.asterisk.org/#/c/5876/11/channels/pjsip/dialplan_functions.c@937
PS11, Line 937: if (!session->pending_media_state->topology) {
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?
https://gerrit.asterisk.org/#/c/5876/11/channels/pjsip/dialplan_functions.c@1008
PS11, Line 1008: if (!data->session->pending_media_state->topology) {
: data->session->pending_media_state->topology = ast_stream_topology_clone(data->session->endpoint->media.topology);
: if (!data->session->pending_media_state->topology) {
: return 0;
: }
: }
Same here
https://gerrit.asterisk.org/#/c/5876/11/channels/pjsip/dialplan_functions.c@1122
PS11, Line 1122: sip_session_response_cb, data->method, 1, NULL);
Should this use the datastore I mentioned above for sending a session refresh?
https://gerrit.asterisk.org/#/c/5876/11/channels/pjsip/include/chan_pjsip.h
File channels/pjsip/include/chan_pjsip.h:
https://gerrit.asterisk.org/#/c/5876/11/channels/pjsip/include/chan_pjsip.h@38
PS11, Line 38: /*!
: * \brief The PJSIP channel driver pvt, stored in the \ref ast_sip_channel_pvt
: * data structure
: */
: struct chan_pjsip_pvt {
: };
Do we just want to go ahead and remove this?
https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_sdp_rtp.c
File res/res_pjsip_sdp_rtp.c:
https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_sdp_rtp.c@944
PS11, Line 944: /* If port is 0, ignore this media stream */
: if (!stream->desc.port) {
: ast_debug(3, "Media stream '%s' is already declined\n",
: ast_codec_media_type2str(session_media->type));
: return 0;
: }
Should this logic actually go into res_pjsip_session so the handler isn't even called?
https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_session.c
File res/res_pjsip_session.c:
https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_session.c@1371
PS11, Line 1371: stream = ast_stream_alloc(ast_codec_media_type2str(type), type);
: if (!stream) {
: return -1;
: }
:
: ast_stream_topology_set_stream(session->pending_media_state->topology, i, stream);
:
: session_media = ast_sip_session_media_state_add(session, session->pending_media_state, ast_media_type_from_str(media), i);
: if (!session_media) {
: return -1;
: }
Document why these don't check for an existing stream or session media
https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_session.c@1681
PS11, Line 1681: ast_stream_topology_free(session->pending_media_state->topology);
This is a noop and should be removed, ast_sip_session_media_state_free does it.
https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_session.c@3408
PS11, Line 3408: if (is_stream_limitation_reached(ast_stream_get_type(stream), session->endpoint, type_streams)) {
: ast_stream_set_state(stream, AST_STREAM_STATE_REMOVED);
: }
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?
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?
https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_t38.c
File res/res_pjsip_t38.c:
https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_t38.c@312
PS11, Line 312: session_media = session->active_media_state->default_session[AST_MEDIA_TYPE_IMAGE];
Should we do anything to the prior active media state we have cloned away to stop RTP or other things?
https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_t38.c@346
PS11, Line 346: ast_stream_set_state(stream, AST_STREAM_STATE_SENDRECV);
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?
https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_t38.c@489
PS11, Line 489: static struct ast_frame *t38_framehook_read(struct ast_channel *chan,
If T.38 is moved to be a proper stream then the use of framehooks can just be limited to the control frames.
https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_t38.c@738
PS11, Line 738: pjmedia_sdp_media *stream = sdp->media[index];
Should this go back to being passed in?
https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_t38.c@890
PS11, Line 890: pjmedia_sdp_media *remote_stream = remote->media[index];
Should this go back to being passed in?
--
To view, visit https://gerrit.asterisk.org/5876
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8afd8dd2eb538806a39b887af0abd046266e14c7
Gerrit-Change-Number: 5876
Gerrit-PatchSet: 11
Gerrit-Owner: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Comment-Date: Wed, 21 Jun 2017 16:57:01 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170621/4b171920/attachment-0001.html>
More information about the asterisk-code-review
mailing list