[Home]

Summary:ASTERISK-15325: [Patch] always m=text 0 in sdp answer
Reporter:peterj (peterj)Labels:
Date Opened:2009-12-16 12:38:06.000-0600Date Closed:2010-01-12 10:21:11.000-0600
Priority:TrivialRegression?No
Status:Closed/CompleteComponents:Channels/chan_sip/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) issue16457.diff
( 1) testlog.txt
( 2) textportinsdp.diff
Description:Hi Issuetracker,
I installed asterisk trunk today, it wasnt working as I was expecting.

I called asterisk with audio, video and text. The Echo() application answered the call. I had videosupport and textsupport enabled and the ulaw,alaw,h263, t140 and t140red codecs enabled in asterisk.

When I typed text it was not being echo until I completed a whole sentence, I suspected that sip message was being used instead of real-time text. This was verified by chan sip logs. From the logs i also saw that asterisk receieved a correct invite and sdp. Asterisk answered wrongly by saying that it wanted text on port 0. Then Ibegan suspecting something was wrong with asterisk chan_sip.c code.

If found this piece of code, its only being executed for video, we need the same for text. If not the result will be that when asterisk calls the function that finds out where rtp media should be sent to, it will never go in to the block where where the text address is setup.

if (add_audio && (p->jointcapability & AST_FORMAT_VIDEO_MASK) && !p->novideo) {
if (p->vrtp) {
 needvideo = TRUE;
 ast_debug(2, "This call needs video offers!\n");
} else
 ast_debug(2, "This call needs video offers, but there's no video support enabled!\n");
}

So, if this code is duplicated and the word 'video' is changed to 'text', then will provide correct port on sdp answer.

Comments:By: peterj (peterj) 2009-12-18 03:19:24.000-0600

I have tested by making an inbound call to asterisk. It is working. Attaching log. Thanks.
By: David Vossel (dvossel) 2010-01-07 16:41:24.000-0600

nice catch!  I looked through previous commits and found why this happens.  The code to do exactly what you're wanting is farther down in the function, but since it is not set where video is, get_our_media_address does not set the text address correctly.

The new patch is pretty much the same thing you did but adds some debugging and removes duplicate code.  Can you verify the new patch works for you?
By: peterj (peterj) 2010-01-12 02:55:25.000-0600

Yep, good that you saw that the code was already there but in the wrong place, I missed that. The new patch is working. Do you need traces or something from me or can we submit the patch and close this issue now?
By: David Vossel (dvossel) 2010-01-12 10:08:15.000-0600

We are good to go, thanks for testing!  I'll get it committed right away
By: Digium Subversion (svnbot) 2010-01-12 10:14:42.000-0600

Repository: asterisk
Revision: 239427

U   trunk/channels/chan_sip.c

------------------------------------------------------------------------
r239427 | dvossel | 2010-01-12 10:14:41 -0600 (Tue, 12 Jan 2010) | 14 lines

fixes text support in sdp answer

The code that handled setting 'm=text' in the sdp was not executing
in the correct order.  The check to see if text was needed came after
the check to add 'm=text' to the sdp, this resulted in 'm=text' always
being set to 0 because it looked like text was never required.

(closes issue ASTERISK-15325)
Reported by: peterj
Patches:
     textportinsdp.diff uploaded by peterj (license 951)
     issue16457.diff uploaded by dvossel (license 671)
Tested by: peterj

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=239427
By: Digium Subversion (svnbot) 2010-01-12 10:16:18.000-0600

Repository: asterisk
Revision: 239428

_U  branches/1.6.2/
U   branches/1.6.2/channels/chan_sip.c

------------------------------------------------------------------------
r239428 | dvossel | 2010-01-12 10:16:18 -0600 (Tue, 12 Jan 2010) | 21 lines

Merged revisions 239427 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

........
 r239427 | dvossel | 2010-01-12 10:14:41 -0600 (Tue, 12 Jan 2010) | 14 lines
 
 fixes text support in sdp answer
 
 The code that handled setting 'm=text' in the sdp was not executing
 in the correct order.  The check to see if text was needed came after
 the check to add 'm=text' to the sdp, this resulted in 'm=text' always
 being set to 0 because it looked like text was never required.
 
 (closes issue ASTERISK-15325)
 Reported by: peterj
 Patches:
       textportinsdp.diff uploaded by peterj (license 951)
       issue16457.diff uploaded by dvossel (license 671)
 Tested by: peterj
........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=239428
By: Digium Subversion (svnbot) 2010-01-12 10:19:38.000-0600

Repository: asterisk
Revision: 239440

_U  branches/1.6.1/
U   branches/1.6.1/channels/chan_sip.c

------------------------------------------------------------------------
r239440 | dvossel | 2010-01-12 10:19:38 -0600 (Tue, 12 Jan 2010) | 21 lines

Merged revisions 239427 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

........
 r239427 | dvossel | 2010-01-12 10:14:41 -0600 (Tue, 12 Jan 2010) | 14 lines
 
 fixes text support in sdp answer
 
 The code that handled setting 'm=text' in the sdp was not executing
 in the correct order.  The check to see if text was needed came after
 the check to add 'm=text' to the sdp, this resulted in 'm=text' always
 being set to 0 because it looked like text was never required.
 
 (closes issue ASTERISK-15325)
 Reported by: peterj
 Patches:
       textportinsdp.diff uploaded by peterj (license 951)
       issue16457.diff uploaded by dvossel (license 671)
 Tested by: peterj
........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=239440
By: Digium Subversion (svnbot) 2010-01-12 10:21:10.000-0600

Repository: asterisk
Revision: 239447

_U  branches/1.6.0/
U   branches/1.6.0/channels/chan_sip.c

------------------------------------------------------------------------
r239447 | dvossel | 2010-01-12 10:21:10 -0600 (Tue, 12 Jan 2010) | 21 lines

Merged revisions 239427 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

........
 r239427 | dvossel | 2010-01-12 10:14:41 -0600 (Tue, 12 Jan 2010) | 14 lines
 
 fixes text support in sdp answer
 
 The code that handled setting 'm=text' in the sdp was not executing
 in the correct order.  The check to see if text was needed came after
 the check to add 'm=text' to the sdp, this resulted in 'm=text' always
 being set to 0 because it looked like text was never required.
 
 (closes issue ASTERISK-15325)
 Reported by: peterj
 Patches:
       textportinsdp.diff uploaded by peterj (license 951)
       issue16457.diff uploaded by dvossel (license 671)
 Tested by: peterj
........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=239447