[Home]

Summary:ASTERISK-11364: [patch] DTMF digits duplicated
Reporter:edgreenberg (edgreenberg)Labels:
Date Opened:2008-02-02 19:26:02.000-0600Date Closed:2008-03-04 12:51:41.000-0600
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Core/Channels
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 0011911.patch
( 1) channel-dtmf-fix-1.4.17.patch
( 2) grandstream-dtmf.txt
( 3) v1-11911.patch
Description:In a seemingly random fashion, DTMF digits, received in RFC2833, are doubled.  This takes place in channel.c. All the action is in __ast_read(). When digits are too short, they are emulated to a suitable length. When sufficient time elapses, they are removed from emulation and passed to __ast_read()'s caller.

There are three places where digits can be "de-emulated." Once, early in the function (about 40 lines in). Secondly, in the large switch statement, under AST_FRAME_NULL. Finally, in the switch statement under AST_FRAME_VOICE.

I determined that if the order of packets ending the DTMF is "end-voice-end-end", the problem occurs. On the other hand, if the order is "end-end-voice-end", or "end-end-end", the problem does not occur.

In the first case, end-voice-end-end, the initial end packet is processed, and __ast_read() enqueues the digit so it can get to be long enough (80 ms). Then a voice frame arrives, and the packet is long enough. The code under case AST_FRAME_VOICE is executed which removes the digit from the queue and sends it up to the caller, but does not turn off the AST_FRAME_EMULATE_DTMF flag. On the next pass, in case AST_FRAME_NULL, the digit is dequeued again, doubling it.

Note that at the very top of case AST_FRAME_VOICE, there is code to clear the flag, but this code is not executed under the conditions that I was experiencing.

This exists in the 1.4 branch as well as in trunk and applying the fix resolved it there as well.

Attached is a patch to resolve this.
Comments:By: edgreenberg (edgreenberg) 2008-02-03 21:55:29.000-0600

Turns out the patch is different between trunk and 1.4.17. I added a second patch.
By: Dmitry Andrianov (dimas) 2008-02-04 09:08:19.000-0600

Well, according to comments, the AST_FLAG_EMULATE_DTMF flag is not reset intentionally when processing FRAME_VOICE. The emulate_dtmf_duration is set to zero instead and on the next voice frame the following code is executed:
               case AST_FRAME_VOICE:
                       /* The EMULATE_DTMF flag must be cleared here as opposed to when the duration
                        * is reached , because we want to make sure we pass at least one
                        * voice frame through before starting the next digit, to ensure a gap
                        * between DTMF digits. */
                       if (ast_test_flag(chan, AST_FLAG_EMULATE_DTMF) && !chan->emulate_dtmf_duration) {
                               ast_clear_flag(chan, AST_FLAG_EMULATE_DTMF);
                               chan->emulate_dtmf_digit = 0;
                       }

So the question is: isn't it a more proper approach to copy the same code to the beginning of case AST_FRAME_NULL ?


By: tbsky (tbsky) 2008-02-17 20:28:27.000-0600

hi:
  my grandstream has dtmf problems.  see grandstream-dtmf.txt. (in the bottom, 12345678 send out as 12245678). i don't know if it is related to this bug. but after applying the patch, the problem seems gone.
  thanks a lot for the patch!!
By: Dmitry Andrianov (dimas) 2008-02-19 00:49:30.000-0600

Guys, could you please test v1-11911.patch .
It requires "svn revert main/channel.c" first unless you have other changes applied.

The patch just makes handling of NULL frames much like handling of VOICE ones (which is known to work).
By: Joshua C. Colp (jcolp) 2008-02-20 13:59:55.000-0600

Could you please confirm whether dimas' patch solves the issue? As well I'm curious to see an rtp debug that shows this...
By: tbsky (tbsky) 2008-02-21 07:00:02.000-0600

hi:
  sorry i am out of office this week and can not doing test. i will try it asap when i back to office.
By: tbsky (tbsky) 2008-03-03 04:06:54.000-0600

hi:
 i use asterisk 1.4.18 with  v1-11740.patch and v1-11911.patch .
the DTMF seems working fine now  for most phones :)
 snom still fails, but that's snom's problem.
By: Digium Subversion (svnbot) 2008-03-03 09:25:13.000-0600

Repository: asterisk
Revision: 105560

U   branches/1.4/main/channel.c

------------------------------------------------------------------------
r105560 | file | 2008-03-03 09:25:09 -0600 (Mon, 03 Mar 2008) | 7 lines

It is possible for no audio to pass between the current digit and next digit so expand logic that clears emulation to AST_FRAME_NULL.
(closes issue ASTERISK-11364)
Reported by: edgreenberg
Patches:
     v1-11911.patch uploaded by dimas (license 88)
Tested by: tbsky

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

http://svn.digium.com/view/asterisk?view=rev&revision=105560
By: Digium Subversion (svnbot) 2008-03-03 09:26:42.000-0600

Repository: asterisk
Revision: 105561

_U  trunk/
U   trunk/main/channel.c

------------------------------------------------------------------------
r105561 | file | 2008-03-03 09:26:39 -0600 (Mon, 03 Mar 2008) | 15 lines

Merged revisions 105560 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r105560 | file | 2008-03-03 11:28:59 -0400 (Mon, 03 Mar 2008) | 7 lines

It is possible for no audio to pass between the current digit and next digit so expand logic that clears emulation to AST_FRAME_NULL.
(closes issue ASTERISK-11364)
Reported by: edgreenberg
Patches:
     v1-11911.patch uploaded by dimas (license 88)
Tested by: tbsky

........

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

http://svn.digium.com/view/asterisk?view=rev&revision=105561
By: Digium Subversion (svnbot) 2008-03-04 11:13:39.000-0600

Repository: asterisk
Revision: 105602

_U  team/group/multiparking/
U   team/group/multiparking/CHANGES
U   team/group/multiparking/channels/chan_local.c
U   team/group/multiparking/channels/chan_sip.c
U   team/group/multiparking/channels/chan_zap.c
U   team/group/multiparking/funcs/func_version.c
U   team/group/multiparking/include/asterisk/_private.h
U   team/group/multiparking/main/asterisk.c
U   team/group/multiparking/main/autoservice.c
U   team/group/multiparking/main/channel.c
U   team/group/multiparking/main/hashtab.c
U   team/group/multiparking/main/pbx.c
U   team/group/multiparking/res/snmp/agent.c

------------------------------------------------------------------------
r105602 | mvanbaak | 2008-03-04 11:13:33 -0600 (Tue, 04 Mar 2008) | 196 lines

Merged revisions 105558,105561,105564,105566,105569,105571,105573-105574,105589-105590,105592-105595,105597 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
r105558 | file | 2008-03-03 16:16:57 +0100 (Mon, 03 Mar 2008) | 14 lines

Merged revisions 105557 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r105557 | file | 2008-03-03 11:15:39 -0400 (Mon, 03 Mar 2008) | 6 lines

Add a comment to describe some logic.
(closes issue ASTERISK-11558)
Reported by: flefoll
Patches:
     chan_sip.c.br14.patch-just-a-comment uploaded by flefoll (license 244)

........

................
r105561 | file | 2008-03-03 16:30:29 +0100 (Mon, 03 Mar 2008) | 15 lines

Merged revisions 105560 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r105560 | file | 2008-03-03 11:28:59 -0400 (Mon, 03 Mar 2008) | 7 lines

It is possible for no audio to pass between the current digit and next digit so expand logic that clears emulation to AST_FRAME_NULL.
(closes issue ASTERISK-11364)
Reported by: edgreenberg
Patches:
     v1-11911.patch uploaded by dimas (license 88)
Tested by: tbsky

........

................
r105564 | russell | 2008-03-03 16:59:50 +0100 (Mon, 03 Mar 2008) | 40 lines

3) In addition to merging the changes below, change trunk back to a regular
  LIST instead of an RWLIST.  The way this list works makes it such that
  a RWLIST provides no additional benefit.  Also, a mutex is needed for
  use with the thread condition.


Merged revisions 105563 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r105563 | russell | 2008-03-03 09:50:43 -0600 (Mon, 03 Mar 2008) | 24 lines

Merge in some changes from team/russell/autoservice-nochans-1.4

These changes fix up some dubious code that I came across while auditing what
happens in the autoservice thread when there are no channels currently in
autoservice.

1) Change it so that autoservice thread doesn't keep looping around calling
  ast_waitfor_n() on 0 channels twice a second.  Instead, use a thread condition
  so that the thread properly goes to sleep and does not wake up until a
  channel is put into autoservice.

  This actually fixes an interesting bug, as well.  If the autoservice thread
  is already running (almost always is the case), then when the thread goes
  from having 0 channels to have 1 channel to autoservice, that channel would
  have to wait for up to 1/2 of a second to have the first frame read from it.

2) Fix up the code in ast_waitfor_nandfds() for when it gets called with no
  channels and no fds to poll() on, such as was the case with the previous code
  for the autoservice thread.  In this case, the code would call alloca(0), and
  pass the result as the first argument to poll().  In this case, the 2nd
  argument to poll() specified that there were no fds, so this invalid pointer
  shouldn't actually get dereferenced, but, this code makes it explicit and
  ensures the pointers are NULL unless we have valid data to put there.

(related to issue ASTERISK-11555)

........

................
r105566 | russell | 2008-03-03 17:02:19 +0100 (Mon, 03 Mar 2008) | 11 lines

Merged revisions 105565 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r105565 | russell | 2008-03-03 10:01:50 -0600 (Mon, 03 Mar 2008) | 3 lines

Update the copyright information for autoservice.  Most of the code in this file
now is stuff that I have written recently ...

........

................
r105569 | russell | 2008-03-03 18:06:35 +0100 (Mon, 03 Mar 2008) | 11 lines

Merged revisions 105568 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r105568 | russell | 2008-03-03 11:05:16 -0600 (Mon, 03 Mar 2008) | 3 lines

Fix a potential memory leak of the local_pvt struct when ast_channel allocation
fails.  Also, in passing, centralize the code necessary to destroy a local_pvt.

........

................
r105571 | russell | 2008-03-03 18:17:27 +0100 (Mon, 03 Mar 2008) | 11 lines

Merged revisions 105570 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r105570 | russell | 2008-03-03 11:16:53 -0600 (Mon, 03 Mar 2008) | 3 lines

In the case of an ast_channel allocation failure, take the local_pvt out of the
pvt list before destroying it.

........

................
r105573 | qwell | 2008-03-03 19:08:05 +0100 (Mon, 03 Mar 2008) | 15 lines

Merged revisions 105572 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r105572 | qwell | 2008-03-03 12:06:52 -0600 (Mon, 03 Mar 2008) | 7 lines

Fix types for astNumChannels and astConfigCallsProcessed.

(closes issue ASTERISK-11553)
Reported by: jeffg
Patches:
     12114.patch uploaded by jeffg (license 192)

........

................
r105574 | russell | 2008-03-03 19:49:34 +0100 (Mon, 03 Mar 2008) | 4 lines

Fix some code that was improperly changed in revision 104866 from issue ASTERISK-11520.

(closes issue ASTERISK-11566, reported by elguero, patched by me)

................
r105589 | russell | 2008-03-04 05:26:39 +0100 (Tue, 04 Mar 2008) | 3 lines

Use ast_copy_string() instead of strncpy(), and use sizeof() instead of
a magic number

................
r105590 | russell | 2008-03-04 05:28:48 +0100 (Tue, 04 Mar 2008) | 3 lines

- Add curly braces around the while loop
- Properly break out of the loop on error when an included context is not found

................
r105592 | russell | 2008-03-04 05:31:53 +0100 (Tue, 04 Mar 2008) | 11 lines

Blocked revisions 105591 via svnmerge

........
r105591 | russell | 2008-03-03 22:31:29 -0600 (Mon, 03 Mar 2008) | 4 lines

Backport a minor bug fix from trunk that I found while doing random code
cleanup.  Properly break out of the loop when a context isn't found when
verify that includes are valid.

........

................
r105593 | russell | 2008-03-04 05:44:28 +0100 (Tue, 04 Mar 2008) | 2 lines

remove unnecessary casts

................
r105594 | russell | 2008-03-04 05:47:32 +0100 (Tue, 04 Mar 2008) | 2 lines

Make it so you don't have to cast away const in a couple places

................
r105595 | russell | 2008-03-04 05:57:29 +0100 (Tue, 04 Mar 2008) | 2 lines

Simplify a trivial snprintf() with ast_copy_string()

................
r105597 | russell | 2008-03-04 17:55:17 +0100 (Tue, 04 Mar 2008) | 2 lines

Update CHANGES heading

................

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

http://svn.digium.com/view/asterisk?view=rev&revision=105602
By: Digium Subversion (svnbot) 2008-03-04 12:51:41.000-0600

Repository: asterisk
Revision: 105728

_U  team/murf/bug6002/
U   team/murf/bug6002/CHANGES
U   team/murf/bug6002/channels/chan_local.c
U   team/murf/bug6002/channels/chan_sip.c
U   team/murf/bug6002/channels/chan_zap.c
U   team/murf/bug6002/funcs/func_version.c
U   team/murf/bug6002/include/asterisk/_private.h
U   team/murf/bug6002/include/asterisk/rtp.h
U   team/murf/bug6002/main/asterisk.c
U   team/murf/bug6002/main/autoservice.c
U   team/murf/bug6002/main/channel.c
U   team/murf/bug6002/main/hashtab.c
U   team/murf/bug6002/main/pbx.c
U   team/murf/bug6002/main/rtp.c
U   team/murf/bug6002/res/snmp/agent.c

------------------------------------------------------------------------
r105728 | murf | 2008-03-04 12:51:34 -0600 (Tue, 04 Mar 2008) | 228 lines

Merged revisions 105558,105561,105564,105566,105569,105571,105573-105574,105589-105590,105592-105595,105597,105675,105677 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
r105558 | file | 2008-03-03 08:16:57 -0700 (Mon, 03 Mar 2008) | 14 lines

Merged revisions 105557 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r105557 | file | 2008-03-03 11:15:39 -0400 (Mon, 03 Mar 2008) | 6 lines

Add a comment to describe some logic.
(closes issue ASTERISK-11558)
Reported by: flefoll
Patches:
     chan_sip.c.br14.patch-just-a-comment uploaded by flefoll (license 244)

........

................
r105561 | file | 2008-03-03 08:30:29 -0700 (Mon, 03 Mar 2008) | 15 lines

Merged revisions 105560 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r105560 | file | 2008-03-03 11:28:59 -0400 (Mon, 03 Mar 2008) | 7 lines

It is possible for no audio to pass between the current digit and next digit so expand logic that clears emulation to AST_FRAME_NULL.
(closes issue ASTERISK-11364)
Reported by: edgreenberg
Patches:
     v1-11911.patch uploaded by dimas (license 88)
Tested by: tbsky

........

................
r105564 | russell | 2008-03-03 08:59:50 -0700 (Mon, 03 Mar 2008) | 40 lines

3) In addition to merging the changes below, change trunk back to a regular
  LIST instead of an RWLIST.  The way this list works makes it such that
  a RWLIST provides no additional benefit.  Also, a mutex is needed for
  use with the thread condition.


Merged revisions 105563 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r105563 | russell | 2008-03-03 09:50:43 -0600 (Mon, 03 Mar 2008) | 24 lines

Merge in some changes from team/russell/autoservice-nochans-1.4

These changes fix up some dubious code that I came across while auditing what
happens in the autoservice thread when there are no channels currently in
autoservice.

1) Change it so that autoservice thread doesn't keep looping around calling
  ast_waitfor_n() on 0 channels twice a second.  Instead, use a thread condition
  so that the thread properly goes to sleep and does not wake up until a
  channel is put into autoservice.

  This actually fixes an interesting bug, as well.  If the autoservice thread
  is already running (almost always is the case), then when the thread goes
  from having 0 channels to have 1 channel to autoservice, that channel would
  have to wait for up to 1/2 of a second to have the first frame read from it.

2) Fix up the code in ast_waitfor_nandfds() for when it gets called with no
  channels and no fds to poll() on, such as was the case with the previous code
  for the autoservice thread.  In this case, the code would call alloca(0), and
  pass the result as the first argument to poll().  In this case, the 2nd
  argument to poll() specified that there were no fds, so this invalid pointer
  shouldn't actually get dereferenced, but, this code makes it explicit and
  ensures the pointers are NULL unless we have valid data to put there.

(related to issue ASTERISK-11555)

........

................
r105566 | russell | 2008-03-03 09:02:19 -0700 (Mon, 03 Mar 2008) | 11 lines

Merged revisions 105565 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r105565 | russell | 2008-03-03 10:01:50 -0600 (Mon, 03 Mar 2008) | 3 lines

Update the copyright information for autoservice.  Most of the code in this file
now is stuff that I have written recently ...

........

................
r105569 | russell | 2008-03-03 10:06:35 -0700 (Mon, 03 Mar 2008) | 11 lines

Merged revisions 105568 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r105568 | russell | 2008-03-03 11:05:16 -0600 (Mon, 03 Mar 2008) | 3 lines

Fix a potential memory leak of the local_pvt struct when ast_channel allocation
fails.  Also, in passing, centralize the code necessary to destroy a local_pvt.

........

................
r105571 | russell | 2008-03-03 10:17:27 -0700 (Mon, 03 Mar 2008) | 11 lines

Merged revisions 105570 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r105570 | russell | 2008-03-03 11:16:53 -0600 (Mon, 03 Mar 2008) | 3 lines

In the case of an ast_channel allocation failure, take the local_pvt out of the
pvt list before destroying it.

........

................
r105573 | qwell | 2008-03-03 11:08:05 -0700 (Mon, 03 Mar 2008) | 15 lines

Merged revisions 105572 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r105572 | qwell | 2008-03-03 12:06:52 -0600 (Mon, 03 Mar 2008) | 7 lines

Fix types for astNumChannels and astConfigCallsProcessed.

(closes issue ASTERISK-11553)
Reported by: jeffg
Patches:
     12114.patch uploaded by jeffg (license 192)

........

................
r105574 | russell | 2008-03-03 11:49:34 -0700 (Mon, 03 Mar 2008) | 4 lines

Fix some code that was improperly changed in revision 104866 from issue ASTERISK-11520.

(closes issue ASTERISK-11566, reported by elguero, patched by me)

................
r105589 | russell | 2008-03-03 21:26:39 -0700 (Mon, 03 Mar 2008) | 3 lines

Use ast_copy_string() instead of strncpy(), and use sizeof() instead of
a magic number

................
r105590 | russell | 2008-03-03 21:28:48 -0700 (Mon, 03 Mar 2008) | 3 lines

- Add curly braces around the while loop
- Properly break out of the loop on error when an included context is not found

................
r105592 | russell | 2008-03-03 21:31:53 -0700 (Mon, 03 Mar 2008) | 11 lines

Blocked revisions 105591 via svnmerge

........
r105591 | russell | 2008-03-03 22:31:29 -0600 (Mon, 03 Mar 2008) | 4 lines

Backport a minor bug fix from trunk that I found while doing random code
cleanup.  Properly break out of the loop when a context isn't found when
verify that includes are valid.

........

................
r105593 | russell | 2008-03-03 21:44:28 -0700 (Mon, 03 Mar 2008) | 2 lines

remove unnecessary casts

................
r105594 | russell | 2008-03-03 21:47:32 -0700 (Mon, 03 Mar 2008) | 2 lines

Make it so you don't have to cast away const in a couple places

................
r105595 | russell | 2008-03-03 21:57:29 -0700 (Mon, 03 Mar 2008) | 2 lines

Simplify a trivial snprintf() with ast_copy_string()

................
r105597 | russell | 2008-03-04 09:55:17 -0700 (Tue, 04 Mar 2008) | 2 lines

Update CHANGES heading

................
r105675 | file | 2008-03-04 11:08:42 -0700 (Tue, 04 Mar 2008) | 16 lines

Merged revisions 105674 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r105674 | file | 2008-03-04 14:05:28 -0400 (Tue, 04 Mar 2008) | 8 lines

When a new source of audio comes in (such as music on hold) make sure the marker bit gets set.
(closes issue ASTERISK-10000)
Reported by: wdecarne
Patches:
     10355.diff uploaded by file (license 11)
(closes issue ASTERISK-10992)
Reported by: kanderson

........

................
r105677 | file | 2008-03-04 11:11:38 -0700 (Tue, 04 Mar 2008) | 10 lines

Merged revisions 105676 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r105676 | file | 2008-03-04 14:10:34 -0400 (Tue, 04 Mar 2008) | 2 lines

In addition to setting the marker bit let's change our ssrc so they know for sure it is a different source.

........

................

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

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