[Home]

Summary:ASTERISK-11264: [patch] refactoring of fax tone detection in DSP
Reporter:Dmitry Andrianov (dimas)Labels:
Date Opened:2008-01-18 20:36:22.000-0600Date Closed:2008-02-20 15:33:23.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:PBX/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) dsp-faxtones.patch
( 1) v2-dsp-faxtones.patch
( 2) v3-dsp-faxtones.patch
( 3) v4-dsp-faxtones.patch
( 4) v5-dsp-faxtones.patch
( 5) v6-dsp-faxtones.patch
Description:Refactored DTMF digit detection, fax tone detection & added CED fax tone detection. See Additional Infrmation for details.

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

1. Added generic code to detect arbitrary frequency of given duration.

2. Fax tone detection reimplemented using the new code and it is not part of DTMF digit detection anymore. Previously to detect fax tone you had to do:

   ast_dsp_set_features(dsp, DSP_FEATURE_DTMF_DETECT | DSP_FEATURE_FAX_DETECT);

eve if you do not want DTMF. Now you can do just:

   ast_dsp_set_features(dsp, DSP_FEATURE_FAX_DETECT);

3. New code is able to detect both CNG and CED signals (the old was only detecting CNG). By default to be compatible with the old code, only CNG is being detected unless one explicitly does:

   ast_dsp_set_faxmode(i->vad, DSP_FAXMODE_DETECT_CED);
or
   ast_dsp_set_faxmode(i->vad, DSP_FAXMODE_DETECT_CNG|DSP_FAXMODE_DETECT_CED);
or
   ast_dsp_set_faxmode(i->vad, DSP_FAXMODE_DETECT_ALL);

Since default value detects CNG only, all users of the code which request DSP_FEATURE_FAX_DETECT should not notice any changes in the DSP code.

4. When CED is detected AST_FRAME_DTMF with subclass='e' is returned.

5. DTMF/MF digit detection refactored a bit - there were identical members in the dtmf_detect_state_t/mf_detect_state_t structures and code for queueing detected digit was copied&pasted. I introduced digit_detect_state_t structure which has all common members and union of the two above.

Comments:By: Jason Parker (jparker) 2008-01-21 16:10:17.000-0600

I looked at this, and it looks pretty good.  I only had a few minor comments.

mhit in dtmf_detect_state_t was renamed to current_hit, but it remains mhit in mf_detect_state_t.  I don't see any reason not to change it in both places.

Could you explain where you got 10% from?  It seems somewhat arbitrary, but perhaps there is a reason.

You say that the fax detection shouldn't change at all, but previously, fax_hits needed to be 5.  Can you explain a little more on how this has changed?


Obviously, this patch is going to require a bit of testing before we would feel comfortable putting it in.  I think answering the above questions would help with that, but I'd certainly like to see "outside" users test this (perhaps you could send a list message requesting that a few people test?)


This is unrelated to this patch, but I happened to notice it while I was reviewing.  I was curious if you were able to explain the MF_GSIZE define, and what is meant by "80 is optimised to meet the MF spec", and more importantly - why it's set to 120, when the allegedly optimized value is 80?

By: Dmitry Andrianov (dimas) 2008-01-21 19:35:21.000-0600

Attached new patch.

1. mhit vs current_hit. I noticed that the mhit was never really used in the MF code even before my modifications. The variable was never assigned a value except of zero. (This is true for MF code only - DTMF code did use mhit).

Anyway, I renamed the variable and also I fixed the code to use it. This however led to another change - how ast_dsp_process interprets result of digit-detection. Now this function is more interested in content of buffer of collected digits than in actual result returned by the digit-detection code.

I believe this is right thing to do. However I clearly understand that it now requires even more testing effort.

2. 10% actually is arbitrary. It is just assumption which looks reasonable.

3. Oh.. You are right. Previously only 5 hits of CNG were required (63ms) and now CNG tone is detected only when it is present for 500ms-10% which means about 450ms.

When I said the old code won't notice anything I actually meant the code will be notified about CNG the same old way (AST_FRAME_DTMF/'f') and won't get unexpected AST_FRAME_DTMF/'e' (CED) notifications. I do not think there is a code which depends on the exact moment when the AST_FRAME_DTMF/'f' gets received relative to tone start.

4. I understand the need of testing. Will do some more testing internally, then ask people I have connections with and then finally will announce on -dev mailing list.

5. Regarding the Goertzel block size for MF. Honestly I have no idea why 120 is used. It means block size is 15ms which is a bit too much when Bell specification minimum tone is 30ms. The 30ms tone starting and ending in the middle of 15ms block won't be detected because code requires two successive hits while first and last blocks will miss. In contrast, with 80 samples block (10ms) the tone would be detected.

Only the original author of the code (steve?) knows why he increased the block size. Probably there too many false positive/negatives.

By: Dmitry Andrianov (dimas) 2008-01-28 02:40:38.000-0600

People who testthe patch, please post your results (successfull or not) here. Thanks.

By: Igor Goncharovsky (igorg) 2008-01-28 22:47:20.000-0600

dimas, second patch doesn't include dsp.h changes. I'll try to test now fax tone detection with mISDN. Report after a while.

By: Dmitry Andrianov (dimas) 2008-01-29 04:31:42.000-0600

IgorG, oh thenks for reporting!

v3 patch does not contain anything new - just the changes for .h file includes,

By: Dmitry Andrianov (dimas) 2008-01-29 13:03:53.000-0600

Updated patch. Fixed bug with DTMF detection reported by IgorG privately.

Also, I have replaced dsp.[ch] on my production 1.4 with patched files from trunk and put these changes into production. So far so good.



By: Igor Goncharovsky (igorg) 2008-01-30 02:26:10.000-0600

Latest version works for me. DTMF, fax CNG and CED successfully detected both on incoming and outgoing calls over mISDN.

By: Dmitry Andrianov (dimas) 2008-01-31 12:11:26.000-0600

Updated patch. The code was looking for the CNG tone even when DSP_FEATURE_FAX_DETECT was not requested.

By: Dmitry Andrianov (dimas) 2008-01-31 17:58:06.000-0600

Ok, I think it is the last update.

Changed selection if Goertzel block size to prefer smaller blocks. There are two reasons:
1. only if we have small enough block it is possible to squelch detected tone from the voice data if we want later. (With large block it is not possible because at the moment tone is detected, it is too late and lots of frames with tone were already passed)
2. More importantly: it makes detection more reliable. This is because:
*) Larger block size allows less deviation of frequency
*) Testing with one of my fax machines i found it regularly produces some clicking sounds which repeated multiple times within the large block makes Goertzel energy for target frequency very low. With small block, that "click" also makes code think there is no tone, but de-bouncing allows these drop-outs in the middle of tone to be handled ok.

By: cache (cache) 2008-02-08 03:40:58.000-0600

Installed patched dsp.c on production 1.4 machine as part of app_fax+T38 installation. No problems noted so far
Good work dimas. Thanks.

By: Digium Subversion (svnbot) 2008-02-20 15:33:21.000-0600

Repository: asterisk
Revision: 103903

U   trunk/include/asterisk/dsp.h
U   trunk/main/dsp.c

------------------------------------------------------------------------
r103903 | qwell | 2008-02-20 15:33:19 -0600 (Wed, 20 Feb 2008) | 13 lines

Largely refactor DSP tone detection routines.

Separate fax detection from digit detected.
Added CED (called) tone detection for fax (previously, only CNG (calling) was supported).
Separate DTMF/MF code paths where appropriate.
Allow detection of arbitary tones.

(closes issue ASTERISK-11264)
Reported by: dimas
Patches:
     v6-dsp-faxtones.patch uploaded by dimas (license 88)
Tested by: dimas, IgorG, Cache

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

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