Summary:	ASTERISK-21184: chan_gulp: Add support for multiple media streams/types
Reporter:	Matt Jordan (mjordan)	Labels:	Asterisk12
Date Opened:	2013-02-27 15:55:32.000-0600	Date Closed:	2013-03-15 08:27:30
Priority:	Major	Regression?
Status:	Closed/Complete	Components:	Channels/chan_pjsip
Versions:	12	Frequency of Occurrence
Related Issues:	must be merged before resolving ASTERISK-21077 Add support for video in chan_gulp
Environment:		Attachments:	( 0) media_rework.diff
Description:	Currently, {{chan_gulp}} only supports audio. While that's nice, we're going to eventually want to have video and fax. And text (why not). Note that while it has larger implications than just the channel driver, the structure of the objects should allow for multiple media streams of a particular type. The actual creation of the media streams can bail if someone requests multiple types, but if the core ever supports such a concept the channel driver should be ready to accept it.
Comments:	By: Kinsey Moore (kmoore) 2013-03-06 16:09:14.356-0600 Initial external testing looks good.  Chan_gulp/res_sip responds properly to SDP with randomly ordered streams and multiples of the same stream types. By: Kinsey Moore (kmoore) 2013-03-07 11:20:09.553-0600 After a little more external testing, chan_gulp/res_sip needs to reject SDP where any two streams have the same non-zero port.  Otherwise, we might accidentally support optional-SRTP. Looking internally, things need to be modified.  The media streams currently reside in a static array on the ast_sip_session (severely restricts new stream types and restricts to one stream of a given type which is currently, but not always desirable) and all registered sdp media type handlers are available to all sessions (prevents disallowing specific stream types on individual endpoints). By: Joshua C. Colp (jcolp) 2013-03-07 11:30:37.143-0600 Yes on rejection. As for the rest: I think it's premature and not worthwhile to modify things to be less static. We'll end up hurting performance, and when the core finally does become aware of multiple media streams it'll most likely be done in such a way where it won't hurt performance. As it is modifying things down the line should be pretty easy in that regard. As for your comment about SDP media type handlers this is true - if the handler isn't fetching configuration from the endpoint to know. How to actually decide I think should be left up to the media type handler. Take video - do we need videosupport=yes? No. If a video codec is configured, then video support is on. By: Kinsey Moore (kmoore) 2013-03-07 13:22:30.743-0600 I agree that multiple streams of the same type is not an issue we need to deal with here and now (or ever, depending on the various directions Asterisk takes).  I also agree that setting handlers available to an endpoint is unnecessary given that we already set codecs. I would prefer to see ast_sip_session_media_position go away entirely and ast_sip_session->media replaced with an ao2_container keyed on stream type ("audio", "video", etc.), but if that is too non-performant it can be done later in a more performant way. On a different note, the code deciding to only allow one audio stream should be in res_sip_session.c, not res_sip_sdp_audio.c By: Joshua C. Colp (jcolp) 2013-03-07 13:25:23.308-0600 Media is sent/received generally every 20ms. That's a hash and lookup twice every 20ms. That's what scares me. By: Kinsey Moore (kmoore) 2013-03-07 13:40:09.751-0600 I see your point.  I had tunnel vision on res/res_sip_*. By: Kinsey Moore (kmoore) 2013-03-09 13:28:55.421-0600 Attached a patch to rework media handlers in res_sip to be a bit more generic and also includes rejection of SDP that uses the same port on multiple streams.  Tested against several scenarios in the SDP_offer_answer integration test. By: Kinsey Moore (kmoore) 2013-03-11 16:49:38.679-0500 Current version up for review: https://reviewboard.asterisk.org/r/2380/ By: Kinsey Moore (kmoore) 2013-03-15 08:27:30.525-0500 This has been commited to the pimp_my_sip group branch.