| Summary: | ASTERISK-20295: Asterisk is not incrementing the sequence numbers for the retransmission of the DTMF end packets(RTPEvent packet with end bit set to 1) | ||||
| Reporter: | NITESH BANSAL (nbansal) | Labels: | |||
| Date Opened: | 2012-08-22 06:52:41 | Date Closed: | 2012-09-04 22:47:07 | ||
| Priority: | Minor | Regression? | |||
| Status: | Closed/Complete | Components: | Resources/res_rtp_asterisk | ||
| Versions: | 1.8.15.0 10.7.0 11.0.0-beta1 | Frequency of Occurrence | Constant | ||
| Related Issues: |
| ||||
| Environment: | OS: Debian squeeze distribution x86_64 architecture | Attachments: | ( 0) 01_rtp_event_seq_num.patch ( 1) asterisk-20295-dtmf-fix-cleanup.diff | ||
| Description: | Issue: All 3 retransmitted ending packets share the same sequence number, which does not comply with RFC 4733 Sect. 2.5.1.6. RTP Sequence Number The RTP sequence number MUST be incremented by one in each successive RTP packet sent. Incrementing applies to retransmitted as well as initial instances of event reports, to permit the receiver to detect lost packets for RTP Control Protocol (RTCP) receiver reports. | ||||
| Comments: | By: NITESH BANSAL (nbansal) 2012-08-22 06:59:50.683-0500 I have fixed the issue on my local setup. Here is the patch file attached. By: Michael L. Young (elguero) 2012-08-23 21:25:37.810-0500 Thanks for your report and contribution. I took a look at your patch and the rfc documentation. There are some problems with your patch. It is not formatted properly. There are comments at the beginning of the patch. Also, you should increment the seqno further down below the debug message or else that message will display the incorrect seqno that was sent. Therefore, I am uploading a patch based on your patch with the appropriate changes. Also, I saw some things that could be cleaned up. Edit: Just noticed that you didn't upload the patch with a license... Edit2: Just found out I could mark it as a contribution :) By: NITESH BANSAL (nbansal) 2012-08-24 03:13:38.390-0500 I would like to add that this issue was found out in the regression testing of an old JIRA issue: ASTERISK-12282 By: Mark Michelson (mmichelson) 2012-08-27 10:41:04.705-0500 A fundamental flaw with this bug report is that it is reporting an RFC violation for an RFC that Asterisk does not claim to support. Asterisk supports transmission and reception of RTP events as described by RFC 2833, not RFC 4733. RFC 2833 is vague regarding whether the sequence number should be incremented for event retransmissions. Is this something that is causing an actual problem for you or is this report strictly being made for the RFC 4733 violation? If there are no actual defects observed, then I do not want to make this change. The RFC 2833 language does not explicitly require incrementing the sequence number for retransmissions. Making the change you suggest has the potential to cause regressions. If Asterisk's behavior is causing an issue with a device of yours, then I still think this new behavior should be optional. I would like to avoid any potential regressions as much as possible. By: NITESH BANSAL (nbansal) 2012-08-27 10:55:28.292-0500 This issue has resurfaced on our side during regression testing of an old JIRA issue 12282 and a fix for it was accepted at that time. Moreover, there is another major violation of basic RTP RFC in the current code of asterisk, it does not even increment the Sequence number of a regular audio packet followed by the DTMF packet, first regular audio packet followed by a DTMF packet carries the same RTP sequence number as that of last DTMF packet sent. The patch uploaded here is fixing both the violations:: RFC4733 violation and basic RTP RFC violation. By: Michael L. Young (elguero) 2012-08-27 12:23:54.038-0500 Hmm... I would be all for an option. Glad you chimed in because I totally spaced it when looking at this. I guess when I looked at it, I figured we should be supporting the latest RFC and that is all I looked at. RFC4733 makes RFC2833 obsolete. Therefore, I would think it would be in the best interest for Asterisk to support RFC4733 since I would hope that developers are developing against the latest standard and not an old one with vague wording. I can see the vagueness... "(The RTP sequence number is incremented by one for each packet.)" ... but it doesn't specify for retransmissions like rfc4733 does. By: Richard Mudgett (rmudgett) 2012-08-27 15:06:42.328-0500 Please use the full issue id when referring to issues. "old JIRA issue 12282" is vague. Do you mean ASTERISK-12282? By: NITESH BANSAL (nbansal) 2012-08-27 15:09:39.299-0500 Hi Richard, yes i meant Asterisk-12282 By: Torrey Searle (tsearle) 2012-08-27 15:17:43.199-0500 Actually, I believe he's referring to ASTERISK-12308 By: NITESH BANSAL (nbansal) 2012-08-27 15:23:32.146-0500 Thanks for the correction torrey. Torrey is right, i am referring to the issue ASTERISK-12308, sorry for the inconvenice, i had the wrong JIRA ID in my list. By: Michael L. Young (elguero) 2012-08-27 15:50:51.311-0500 Now things are becoming a bit clearer... I was wondering how the other issue was related. So, it looks like the proposed change was already done in 1.4... I checked 1.6.1 and 1.6.2 and it is present there... but the introduction of res_rtp_asterisk.c in 1.8 brought the regression in. By: i2045 (i2045) 2012-08-28 10:22:12.846-0500 "Moreover, there is another major violation of basic RTP RFC in the current code of asterisk, it does not even increment the Sequence number of a regular audio packet followed by the DTMF packet, first regular audio packet followed by a DTMF packet carries the same RTP sequence number as that of last DTMF packet sent." The behaviour of invalid RTP sequence numbers is something i also noticed when Asterisk is "injecting" RTP DTMF events in between RTP audio packets. See ASTERISK-19821 for a packet capture of this. By: Mark Michelson (mmichelson) 2012-08-30 08:08:45.654-0500 Okay, if 1.4 and 1.6.1 and 1.6.2 had this behavior, I'm willing to change it in 1.8 forward without the need for an option. I'll do a review of Michael's patch on review board today. By: Mark Michelson (mmichelson) 2012-08-30 09:07:29.554-0500 I've given a "Ship it!" to Michael's review. You can commit when ready. | ||||