[asterisk-commits] mmichelson: branch mmichelson/correct_sdp_answer r206498 - /team/mmichelson/c...

[SVN commits to the Asterisk project]

Author: mmichelson
Date: Tue Jul 14 12:18:18 2009
New Revision: 206498

URL: http://svn.asterisk.org/svn-view/asterisk?view=rev&rev=206498
Log:
Added some notes while looking over the SDP parsing and generating functions.


Modified:
    team/mmichelson/correct_sdp_answer/channels/chan_sip.c

Modified: team/mmichelson/correct_sdp_answer/channels/chan_sip.c
URL: http://svn.asterisk.org/svn-view/asterisk/team/mmichelson/correct_sdp_answer/channels/chan_sip.c?view=diff&rev=206498&r1=206497&r2=206498
==============================================================================
--- team/mmichelson/correct_sdp_answer/channels/chan_sip.c (original)
+++ team/mmichelson/correct_sdp_answer/channels/chan_sip.c Tue Jul 14 12:18:18 2009
@@ -5308,6 +5308,11 @@
 
 		numberofports = 1;
 		len = -1;
+		/* XXX MCM It appears that if we come across multiple audio streams, then the final one
+		 * we parse is the one that will actually be answered in the answering SDP.
+		 *
+		 * Same appears to apply to video and image streams, too.
+		 */
 		if ((sscanf(m, "audio %d/%d RTP/AVP %n", &x, &numberofports, &len) == 2 && len > 0) ||
 		    (sscanf(m, "audio %d RTP/AVP %n", &x, &len) == 1 && len > 0)) {
 			audio = TRUE;
@@ -5324,6 +5329,11 @@
 					ast_verbose("Found RTP audio format %d\n", codec);
 				ast_rtp_set_m_type(newaudiortp, codec);
 			}
+		/* XXX MCM It appears that video streams are parsed on incoming offers no matter whether video
+		 * support is enabled or not. This is a good thing with regards to this branch. The information
+		 * is stored in the "newvideortp" structure. I'll need to check down below to see what is actually
+		 * done with this info...
+		 */
 		} else if ((sscanf(m, "video %d/%d RTP/AVP %n", &x, &numberofports, &len) == 2 && len > 0) ||
 		    (sscanf(m, "video %d RTP/AVP %n", &x, &len) == 1 && len >= 0)) {
 			/* If it is not audio - is it video ? */
@@ -5739,6 +5749,16 @@
 	p->jointnoncodeccapability = newnoncodeccapability;	/* DTMF capabilities */
 
 	ast_rtp_pt_copy(p->rtp, newaudiortp);
+	/* XXX MCM. Ah cripes, so even though we're storing the video information in the newvideortp
+	 * structure, we don't actually store anything about it on p unless there is a vrtp. vrtp's existence
+	 * is predicated on "videosupport" being enabled in sip.conf.
+	 *
+	 * So something needs to be added to this function so we can store a simple boolean on p telling if
+	 * video was offered. That way, in add_sdp, we can use this info to tell if we need to add an m= line
+	 * for video even if video support is not enabled.
+	 *
+	 * Similar measures need to be taken for image and audio streams as well.
+	 */
 	if (p->vrtp)
 		ast_rtp_pt_copy(p->vrtp, newvideortp);
 
@@ -6746,6 +6766,10 @@
 
 		/* Ok, we need video. Let's add what we need for video and set codecs.
 		   Video is handled differently than audio since we can not transcode. */
+		/* XXX MCM To simplify matters, a good idea would probably be to use whatever boolean was stored
+		 * on p to set the 'needvideo' variable. Then, within this block, we can just set vdest.sin_port to
+		 * 0.
+		 */
 		if (needvideo) {
 			/* Determine video destination */
 			if (p->vredirip.sin_addr.s_addr) {
@@ -6809,6 +6833,9 @@
 		}
 
 		/* Now send any other common audio and video codecs, and non-codec formats: */
+		/* XXX MCM Will need to make sure that we don't actually start adding video codecs if 
+		 * video is not supported.
+		 */
 		for (x = 1; x <= (needvideo ? AST_FORMAT_MAX_VIDEO : AST_FORMAT_MAX_AUDIO); x <<= 1) {
 			if (!(capability & x))	/* Codec not requested */
 				continue;
@@ -6859,6 +6886,11 @@
 			ast_build_string(&m_video_next, &m_video_left, "\r\n");
 	}
 
+	/* XXX MCM the 'add_t38' variable is determined by the caller of the function and will be set true
+	 * if we wish to offer/answer t38 for this stream. We need to be prepared to place t38 streams with
+	 * port set to 0 even if the caller of this function is not going to request t38 be added if the offer
+	 * we are answering had t38 in it.
+	 */
 	if (add_t38 && p->udptl) {
 		struct sockaddr_in udptlsin;
 		struct sockaddr_in udptldest = { 0, };