[Home]

Summary:ASTERISK-15559: [patch] DSP progress detection unable to detect SIT
Reporter:dant (dant)Labels:
Date Opened:2010-02-02 00:34:37.000-0600Date Closed:2010-05-19 01:41:58
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) dsp.c-bug16749-1.patch
Description:The DSP should be able to detect the SIT. The existing code checks for the first tone followed by the second tone, followed by the third tone in sequence and on detection of the third tone allows firing a CONGESTION control frame.

This unfortunately doesn't work due to the fact that entire SIT is about 1 second long, the DSP is working with smaller samples and the code will not stay in a second or third tone state beyond one pass.

This issue is being raised against trunk, but, the problem appears to be present even in 1.2.

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

They key bit of code is...
<snip>
} else if (hz[HZ_950] > TONE_MIN_THRESH * TONE_THRESH) {
   newstate = DSP_TONE_STATE_SPECIAL1;
} else if (hz[HZ_1400] > TONE_MIN_THRESH * TONE_THRESH) {
   if (dsp->tstate == DSP_TONE_STATE_SPECIAL1)
       newstate = DSP_TONE_STATE_SPECIAL2;
} else if (hz[HZ_1800] > TONE_MIN_THRESH * TONE_THRESH) {
   if (dsp->tstate == DSP_TONE_STATE_SPECIAL2) {
       newstate = DSP_TONE_STATE_SPECIAL3;
   }
<snip>
} else {
       newstate = DSP_TONE_STATE_SILENCE;
}
<snip>

When a SIT is present, the DSP processes each frame, it detects the 950Hz tone for 16 cycles and stays in DSP_TONE_STATE_SPECIAL1 state for that time, then the 1400Hz tone starts and it switches DSP_TONE_STATE_SPECIAL2 due to the 1400Hz tone being present and the previous state being DSP_TONE_STATE_SPECIAL1, however, on the next cycle the 1400Hz tone is still present and the current state is DSP_TONE_STATE_SPECIAL2, so, the default state of DSP_TONE_STATE_SILENCE is set and the sequence can never complete.
Comments:By: dant (dant) 2010-02-02 00:37:21.000-0600

Relevant debug output from reception of two SITs a couple of seconds apart:
[Feb  2 13:25:42] DEBUG[10497] dsp.c: Stop state 0 with duration 0
[Feb  2 13:25:42] DEBUG[10497] dsp.c: Start state 5
[Feb  2 13:25:42] DEBUG[10497] dsp.c: Stop state 5 with duration 15
[Feb  2 13:25:42] DEBUG[10497] dsp.c: Start state 6
[Feb  2 13:25:42] DEBUG[10497] dsp.c: Stop state 6 with duration 1
[Feb  2 13:25:42] DEBUG[10497] dsp.c: Start state 0
[Feb  2 13:25:45] DEBUG[10497] dsp.c: Stop state 0 with duration 121
[Feb  2 13:25:45] DEBUG[10497] dsp.c: Start state 5
[Feb  2 13:25:45] DEBUG[10497] dsp.c: Stop state 5 with duration 16
[Feb  2 13:25:45] DEBUG[10497] dsp.c: Start state 6
[Feb  2 13:25:45] DEBUG[10497] dsp.c: Stop state 6 with duration 1
[Feb  2 13:25:45] DEBUG[10497] dsp.c: Start state 0

By: dant (dant) 2010-02-02 00:43:32.000-0600

Patch attached that allows the second and third tones to be present across multiple dsp calls allowing the sequence to complete...

New debug output:
[Feb  2 14:10:20] DEBUG[11515] dsp.c: Stop state 0 with duration 0
[Feb  2 14:10:20] DEBUG[11515] dsp.c: Start state 5
[Feb  2 14:10:21] DEBUG[11515] dsp.c: Stop state 5 with duration 15
[Feb  2 14:10:21] DEBUG[11515] dsp.c: Start state 6
[Feb  2 14:10:21] DEBUG[11515] dsp.c: Stop state 6 with duration 15
[Feb  2 14:10:21] DEBUG[11515] dsp.c: Start state 7

By: Tilghman Lesher (tilghman) 2010-05-18 14:42:34

The patch attached does not do what you indicate above.  In fact, it doesn't do anything.  I would ascribe any testing results to be purely coincidental or could be related to another change you've made.

By: dant (dant) 2010-05-18 18:27:41

tilghman,
Are you really sure that that is the case? This was the only change to dsp.c...

Looking at the code again, this appears to be called for each frame, 20ms long... An SIT, as you know, is made up of approx 330ms of 950hz, followed by approx 330ms of 1400hz, followed by approx 300ms of 1800hz... So, for the first frame of SIT, 950hz would be detected and newstate would be set to 5 (DSP_TONE_STATE_SPECIAL1), for the subsequent 15 frames of 950hz newstate would continue to be set to 5, on line 1023 newstate would be compared to the current state (dsp->tstate) and if it was the same, dsp->tcount would be incremented, so, reaching 15... for the first frame that 1400hz is detected, the current state would be first checked to make sure it was 5, if it was, newstate would be set to 6 (DSP_TONE_STATE_SPECIAL2), for the second frame that 1400hz was detected the current state would be 6, so the check to make sure it was 5 would fail, and newstate would not be set. At line 1023 dsp->tstate would not equal newstate as newstate would still have it's initialised value of 0 (DSP_TONE_STATE_SILENCE) due to the fact that while 1400hz was found, the current state was not 5, as such, at line 1071 the debug messages above would be output and the current state would be set to newstate, in this case 0. There could never be a way for the switch statement on line 1028 to ever match on a current state of 7 even if the SIT was 3 frames long...

The patch does change this behavior, on the second frame of 1400hz being detected the enclosed if statement would evaluate true as the current state of 6 does match 5 || 6... as a result, at line 1023 it would find that the state hadn't changed, it wouldn't reset the state to 0 as line 1071 would never by hit and for all subsequent instances of 1400hz newstate would continue to be set to 6 allowing for an eventual setting of newstate to 7 (DSP_TONE_STATE_SPECIAL3) when the first 1800hz frame was detected such that on the second frame the switch statement on line 1028 would match a current state of 7, and on the 4th frame of that the threshold would be reached and res would be set to AST_CONTROL_CONGESTION...
To me, that does indeed seem to resolve the logic problem with the current code...

Thanks...

By: Tilghman Lesher (tilghman) 2010-05-19 01:17:51

Ah, I think I see.  You're referring to a much lower if clause than I was looking at.

By: Digium Subversion (svnbot) 2010-05-19 01:32:27

Repository: asterisk
Revision: 263949

U   branches/1.4/main/dsp.c

------------------------------------------------------------------------
r263949 | tilghman | 2010-05-19 01:32:27 -0500 (Wed, 19 May 2010) | 8 lines

Because progress is called multiple times, across several frames, we must persist states when detecting multitone sequences.

(closes issue ASTERISK-15559)
Reported by: dant
Patches:
      dsp.c-bug16749-1.patch uploaded by dant (license 670)
Tested by: dant

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

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

By: Digium Subversion (svnbot) 2010-05-19 01:41:04

Repository: asterisk
Revision: 263950

_U  trunk/
U   trunk/main/dsp.c

------------------------------------------------------------------------
r263950 | tilghman | 2010-05-19 01:41:04 -0500 (Wed, 19 May 2010) | 15 lines

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

........
 r263949 | tilghman | 2010-05-19 01:32:27 -0500 (Wed, 19 May 2010) | 8 lines
 
 Because progress is called multiple times, across several frames, we must persist states when detecting multitone sequences.
 
 (closes issue ASTERISK-15559)
  Reported by: dant
  Patches:
        dsp.c-bug16749-1.patch uploaded by dant (license 670)
  Tested by: dant
........

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

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

By: Digium Subversion (svnbot) 2010-05-19 01:41:58

Repository: asterisk
Revision: 263951

_U  branches/1.6.2/
U   branches/1.6.2/main/dsp.c

------------------------------------------------------------------------
r263951 | tilghman | 2010-05-19 01:41:57 -0500 (Wed, 19 May 2010) | 22 lines

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

................
 r263950 | tilghman | 2010-05-19 01:41:04 -0500 (Wed, 19 May 2010) | 15 lines
 
 Merged revisions 263949 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r263949 | tilghman | 2010-05-19 01:32:27 -0500 (Wed, 19 May 2010) | 8 lines
   
   Because progress is called multiple times, across several frames, we must persist states when detecting multitone sequences.
   
   (closes issue ASTERISK-15559)
    Reported by: dant
    Patches:
          dsp.c-bug16749-1.patch uploaded by dant (license 670)
    Tested by: dant
 ........
................

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

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