[Home]

Summary:ASTERISK-12282: suspected typo in main/rtp.c bridge_p2p_rtp_write() payload type check (can cause RFC2833 DTMF detection issues)
Reporter:tonyredstone (tonyredstone)Labels:
Date Opened:2008-06-30 10:44:25Date Closed:2008-07-09 14:25:05
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Core/RTP
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) dynamic_rtp.patch
Description:Hi,

Near the top of bridge_p2p_rtp_write(), there is code that reads as follows:
     /* If the payload coming in is not one of the negotiated ones
then send it to the core, this will cause formats to change and the
bridge to break */
      if (!bridged->current_RTP_PT[payload].code)
              return -1;

The value of payload is from the inbound leg, however, (struct ast_rtp *) bridged is the RTP struct for the *outbound* leg.  I believe this code should read:
      /* If the payload coming in is not one of the negotiated ones
then send it to the core, this will cause formats to change and the
bridge to break */
      if (!rtp->current_RTP_PT[payload].code)
              return -1;

since (struct ast_rtp *)rtp is the structure for the inbound leg so now the code matches the comment (and it makes sense).

With the code as it currently stands, packet2packet briding sometimes punts packets to the core unnecessarily and can cause DTMF detection breakage under certain conditions (see below).

Regards,
-Tony.

patch:
--- main/rtp.c.orig 2008-05-14 22:32:00.000000000 +0100
+++ main/rtp.c 2008-06-20 15:48:44.000000000 +0100
@@ -1063,7 +1063,7 @@
rtpPT = ast_rtp_lookup_pt(rtp, payload);

/* If the payload coming in is not one of the negotiated ones then
send it to the core, this will cause formats to change and the bridge
to break */
- if (!bridged->current_RTP_PT[payload].code)
+ if (!rtp->current_RTP_PT[payload].code)
return -1;

/* If the payload is DTMF, and we are listening for DTMF - then feed
it into the core */

****** ADDITIONAL INFORMATION ******

In the case of a SIP call passing through an asterisk 1.4 box, if the inbound leg has negotiated a different telephony-event payload type than the outbound leg, the check above will return true when RFC2833 RTP packets are received and they will be punted to the core.  This means that the core regenerates the rfc2833 packets and they are therefore given a different RTP SSRCID than the p2p bridged RTP that doesn't pass through the core.   This creates DTMF detection issues further upstream because the RTP audio and RTP RFC2833 now have different SSRC IDs (observed in wireshark traces).  Applying the patch above resolves the issue (and wireshark traces show a consistent SSRC id).

As a practical example of the above, Thomson Speedtouch 780WL routers use payload type 97 for telephony-event whereas Asterisk uses 101.  Therefore, SIP calls made from a speedtouch 780 router through an asterisk 1.4 box to an IVR app further upstream (eg BT call centre) will find DTMF detection doesn't work.

For further validation of this, we used Linksys SIP phone which permits you to configure the telephony-event payload type it uses.  It defaults to 101 and, with this configuration, it works.  If we change it to 97, DTMF detection further upstream breaks in the same way as it does when using a speedtouch 780WL router.  After applying the patch above, the DTMF detection when dialing out with  Linksys phone set to 97 for telephony-event then works.

Asterisk platform is 1.4.21 on Debian Etch (4.0), i386 architecture.
Comments:By: tonyredstone (tonyredstone) 2008-06-30 10:49:36

The issue is also present in the latest SVN revision r126572.
By: Torrey Searle (tsearle) 2008-07-04 07:20:36

Some clarification about the code

It isn't a typo.  The method sends RTP packets to "bridged" thus it is checking if "bridged" is actually supports the codec that is trying to be sent.  If it does not, then clearly transcoding needs to occur.  (In your case, the payload type needs to be changed from 97 to 101 before forwarding)

Your change prevents this logic from occurring, which can lead to one way audio situations.
By: tonyredstone (tonyredstone) 2008-07-09 05:06:27

Hi,

That's not how I read the code...!

Let's step through:
=
       int res = 0, payload = 0, bridged_payload = 0, mark;
       struct rtpPayloadType rtpPT;
       int reconstruct = ntohl(rtpheader[0]);

       /* Get fields from packet */
       payload = (reconstruct & 0x7f0000) >> 16;
       mark = (((reconstruct & 0x800000) >> 23) != 0);

       /* Check what the payload value should be */
       rtpPT = ast_rtp_lookup_pt(rtp, payload);
=

so by this point, "payload" is set to the value of the payload type received on the inbound leg.  Also, rtpPT is set to an asterisk internal representation of what the payload type refers to.  Agreed?

the next couple of lines:
=
 /* If the payload coming in is not one of the negotiated ones
then send it to the core, this will cause formats to change and the
bridge to break */
      if (!bridged->current_RTP_PT[payload].code)
              return -1;
=
recall payload is the value of the payload type on the receiving (inbound) side.  However, here it's used to lookup whether the same payload type number happens to be in use on the *outbound* leg.  This is nonsensical because dynamic payload type numbers are only significant within a particular leg of the call.  To put it another way, there's no reason why identical RTP media formats (which use dynamic pt numbers) cannot have completely different payload type numbers on different legs of the call.

let's move on:
=
       /* If the payload is DTMF, and we are listening for DTMF - then feed it into the core */
       if (ast_test_flag(rtp, FLAG_P2P_NEED_DTMF) && !rtpPT.isAstFormat && rtpPT.code == AST_RTP_DTMF)
               return -1;

       /* Otherwise adjust bridged payload to match */
       bridged_payload = ast_rtp_lookup_code(bridged, rtpPT.isAstFormat, rtpPT.code);
=

The last line here will do the inbound pt -> outbound pt number translation.

One further point, if transcoding needs to occur, then the two calls legs will not be packet2packet bridged in the first place (see ast_rtp_bridge()).

It occurs to me that the if (!bridged->current_RTP_PT[payload].code) test would be valid if it also checked that payload<96, ie is in the static payload type range.

Whilst I see more value in changing the check from being against bridged to rtp, if, instead, we added a payload type range check so that we're only testing static pt numbers, I suspect it would still solve the DTMF detection issue caused by asterisk regenerating rfc2833 tones with a different SSRC id from the RTP media.

so the test could look like this:
if (payload<96 && !bridged->current_RTP_PT[payload].code)
              return -1;
By: Torrey Searle (tsearle) 2008-07-09 05:32:30

There are cases where the P2P bridge can be invoked and needs to be shut down.

the P2P bridge is started so long as there is at least one codec in common between the two end points

Example

A: (GSM, G7.11)  calls B: (G711 only)

Asterisk will see that both can do G711 and start the bridge
if A sends GSM (which was still allowed in the 200 OK), it will be forwarded without transcoding and be ignored by B (one way audio)


The line of code will see that B can't do GSM so it will set up the transcoding path

I agree that dynamic payloads were not taken into account in the cited logic.  I'm attaching a patch that should take the dynamic payloads into account.  Could you test to see if this resolves your issue?
By: tonyredstone (tonyredstone) 2008-07-09 05:40:13

I see, makes sense.  The patch looks like it should solve the issue, I'll test and get back to you.
By: tonyredstone (tonyredstone) 2008-07-09 06:38:04

Hi,

I can confirm that dynamic_rtp.patch also resolves the DTMF detection issue (and wireshark shows me that the SSRC IDs on the outbound leg for rtp media and rfc2833 signalling are now the same).

To verify the validity of my test, I reverted to stock 1.4.21 and the issue reoccurs.

Thankyou!
By: Mark Michelson (mmichelson) 2008-07-09 13:58:58

The patch seems fine by me and by file, so I'll be merging it on up. Thanks for your time and analysis on this, folks.
By: Digium Subversion (svnbot) 2008-07-09 14:24:52

Repository: asterisk
Revision: 129436

U   branches/1.4/main/rtp.c

------------------------------------------------------------------------
r129436 | mmichelson | 2008-07-09 14:24:48 -0500 (Wed, 09 Jul 2008) | 13 lines

Fix a problem where inbound rfc2833 audio would be sent to the
core instead of being P2P bridged. When the core regenerated
the rfc2833 packet for the outbound leg, the SSRC would be different
than the RTP audio on the call leg causing DTMF detection issues on
the far end.

(closes issue ASTERISK-12282)
Reported by: tonyredstone
Patches:
     dynamic_rtp.patch uploaded by tsearle (license 373)
Tested by: tonyredstone


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

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