[Asterisk-code-review] chan pjsip: Add support for multiple streams of the same type. (asterisk[master])
Richard Mudgett
asteriskteam at digium.com
Wed Jun 21 15:36:02 CDT 2017
Richard Mudgett 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
(33 comments)
https://gerrit.asterisk.org/#/c/5876/11//COMMIT_MSG
Commit Message:
https://gerrit.asterisk.org/#/c/5876/11//COMMIT_MSG@36
PS11, Line 36: within the resulting the SDP.
within the resulting SDP.
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@523
PS11, Line 523: if (!(pvt = ao2_alloc(sizeof(*pvt), NULL))) {
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.
* Maybe we should add a dummy member so the struct isn't empty.
* Was the ao2 lock ever needed for this struct?
https://gerrit.asterisk.org/#/c/5876/11/channels/chan_pjsip.c@565
PS11, Line 565: if (!topology) {
: ao2_ref(caps, -1);
: ast_channel_unlock(chan);
: ast_hangup(chan);
: return NULL;
: }
Should be:
if (!topology || !caps) {
ao2_cleanup(caps);
ast_stream_topology_free(topology);
ast_channel_unlock(chan);
ast_hangup(chan);
return NULL;
}
https://gerrit.asterisk.org/#/c/5876/11/channels/chan_pjsip.c@572
PS11, Line 572: ast_channel_set_stream_topology(chan, topology);
This probably should be done after stage snapshot with the ast_channel_native_formats_set() call since it is similar.
https://gerrit.asterisk.org/#/c/5876/11/channels/chan_pjsip.c@574
PS11, Line 574: if (!caps) {
: ast_channel_unlock(chan);
: ast_hangup(chan);
: return NULL;
: }
This can now be deleted with above change.
https://gerrit.asterisk.org/#/c/5876/11/channels/chan_pjsip.c@881
PS11, Line 881: if (stream_num >= 0) {
Should the else clause get the default stream since chan_pjsip_write() calls with -1?
https://gerrit.asterisk.org/#/c/5876/11/channels/chan_pjsip.c@887
PS11, Line 887: if (!media) {
: return 0;
: }
This check is redundant with the switch cases.
https://gerrit.asterisk.org/#/c/5876/11/channels/chan_pjsip.c@897
PS11, Line 897: ast_log(LOG_WARNING, "Channel %s stream %d is of type '%s', not audio!\n",
: ast_channel_name(ast), stream_num, ast_codec_media_type2str(media->type));
I suspect this warning and the same warning in video will spew spam when stream types change.
https://gerrit.asterisk.org/#/c/5876/11/channels/chan_pjsip.c@1418
PS11, Line 1418: AST_VECTOR_CALLBACK_VOID(&session->active_media_state->sessions, local_hold_set_state, held);
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.
https://gerrit.asterisk.org/#/c/5876/11/channels/chan_pjsip.c@1441
PS11, Line 1441: struct ast_stream_topology *topology;
This member doesn't appear to be used.
https://gerrit.asterisk.org/#/c/5876/11/channels/chan_pjsip.c@1486
PS11, Line 1486: } else {
: /* The topology change failed, so drop the current pending media state */
What about 1xx responses? Shouldn't they be ignored?
https://gerrit.asterisk.org/#/c/5876/11/channels/chan_pjsip.c@1497
PS11, Line 1497: struct topology_change_refresh_data *refresh_data = data;
:
: if (ast_sip_session_refresh(refresh_data->session, NULL, NULL, on_topology_change_response,
: AST_SIP_SESSION_REFRESH_METHOD_INVITE, 1, refresh_data->media_state)) {
: refresh_data->media_state = NULL;
: topology_change_refresh_data_free(refresh_data);
: return -1;
: }
:
: refresh_data->media_state = NULL;
: topology_change_refresh_data_free(refresh_data);
: return 0;
This could be:
{
struct topology_change_refresh_data *refresh_data = data;
int ret;
ret = ast_sip_session_refresh(...);
refresh_data->media_state = NULL;
topology_change_refresh_data_fress(refresh_data);
return ret;
}
https://gerrit.asterisk.org/#/c/5876/11/channels/pjsip/cli_commands.c
File channels/pjsip/cli_commands.c:
https://gerrit.asterisk.org/#/c/5876/11/channels/pjsip/cli_commands.c@339
PS11, Line 339: static int find_audio(struct ast_sip_session_media *session_media)
Should be NULL tolerant of session_media.
https://gerrit.asterisk.org/#/c/5876/11/channels/pjsip/cli_commands.c@369
PS11, Line 369: ast_channel_lock(channel);
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.
The session->active_media_state->sessions vector likely has reentrancy problems that were prevented by the old media ao2 container lock.
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?
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.
https://gerrit.asterisk.org/#/c/5876/11/include/asterisk/res_pjsip_session.h
File include/asterisk/res_pjsip_session.h:
https://gerrit.asterisk.org/#/c/5876/11/include/asterisk/res_pjsip_session.h@354
PS11, Line 354: * \param session_media The media session
Spaces were used to indent here.
https://gerrit.asterisk.org/#/c/5876/11/include/asterisk/res_pjsip_session.h@387
PS11, Line 387: * \param session_media The media session
Spaces were used to indent here.
https://gerrit.asterisk.org/#/c/5876/11/main/channel.c
File main/channel.c:
https://gerrit.asterisk.org/#/c/5876/11/main/channel.c@10955
PS11, Line 10955: if (ast_stream_topology_equal(ast_channel_get_stream_topology(chan), topology)) {
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.
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@a1384
PS11, Line 1384:
We seem to have lost this declined stream check. Is it really no longer needed?
https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_sdp_rtp.c@1161
PS11, Line 1161: /* If this is a removed (or declined) stream OR if no formats exist then construct a minimal stream in SDP */
: if (ast_stream_get_state(stream) == AST_STREAM_STATE_REMOVED || !ast_stream_get_formats(stream) ||
: !ast_format_cap_count(ast_stream_get_formats(stream))) {
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.
https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_sdp_rtp.c@1164
PS11, Line 1164: media->desc.transport = STR_RTP_AVP;
We should use the same transport as the remote if we are declining a remote offered stream.
https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_sdp_rtp.c@1166
PS11, Line 1166: media->desc.port_count = 1;
Spaces used to indent.
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@306
PS11, Line 306: return AST_VECTOR_APPEND(&session->pending_media_state->read_callbacks, callback_state);
* Probably should have a comment that the vector elements are structs and not pointers to a struct.
* Probably should emphasize that where the vector is declared as well.
https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_session.c@399
PS11, Line 399: session_media = ao2_alloc(sizeof(*session_media), session_media_dtor);
Is the ao2 lock needed?
https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_session.c@414
PS11, Line 414: if (ast_stream_get_state(ast_stream_topology_get_stream(media_state->topology, position)) != AST_STREAM_STATE_REMOVED &&
: !media_state->default_session[type]) {
We can avoid some API calls by swapping the order of the && expressions: B && A instead of A && B
https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_session.c@422
PS11, Line 422: static int is_stream_limitation_reached(enum ast_media_type type, const struct ast_sip_endpoint *endpoint, int *type_streams)
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.
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.
https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_session.c@430
PS11, Line 430: case AST_MEDIA_TYPE_TEXT:
: /* We don't have an option for image (T.38) and text streams
: * so we cap these at one
: */
* AFAIK TEXT streams are not supported by res_pjsip so it should always be hitting the limit like with UNKNOWN.
* The comment should be updated accordingly.
https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_session.c@699
PS11, Line 699: media_state = session->active_media_state;
: session->active_media_state = session->pending_media_state;
:
: /* Active and pending flip flop as needed, so clear the former active for the next pending change */
: session->pending_media_state = media_state;
: sip_session_media_state_reset(session->pending_media_state);
This is broken up strangely. How about:
/* Active and pending flip flop as needed... */
SWAP(session->active_media_state, session->pending_media_state);
sip_session_media_state_reset(session->pending_media_state);
And delete the no longer used media_state local variable.
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
No, this is a use after free and ast_sip_session_media_state_free() currently doesn't free the topology.
https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_session.c@2132
PS11, Line 2132: ast_stream_set_formats(clone_stream, joint_cap);
joint_cap is ref leaked after ast_stream_set_format().
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@300
PS11, Line 300: if (status.code == 100) {
: return 0;
: }
Ha. This callback does get 1xx responses that should be ignored. I pinged this elsewhere and this is confirmation that we need to.
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 t
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.
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 ca
Yes. I think a NULL or empty stream caps shouldn't be allowed if the stream is not declined.
--
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-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Wed, 21 Jun 2017 20:36:02 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170621/4ba6c7dc/attachment-0001.html>
More information about the asterisk-code-review
mailing list